Add AsRef<Path> to eq_file #67
Conversation
epage
left a comment
epage
left a comment
There was a problem hiding this comment.
Thanks for doing this!
Couple of notes
- When we eventually squash the commits, remember to clean up the commit messages to focus on what problem we are solving and any "why"s and not to focus on the "what", that is what the code is for
- Be sure to reference #62 in the commit message
- What is the title of the PR about? I don't understand how it relates to your changes.
src/path/fs.rs
Outdated
| use Predicate; | ||
|
|
||
| fn read_file(path: &path::Path) -> io::Result<Vec<u8>> { | ||
| fn read_file<P: AsRef<Path>>(path: P) -> io::Result<Vec<u8>> { |
There was a problem hiding this comment.
Only the top-level function should do this.
Its best to minimize what all code lives in generics because the compiler will "monomorphize" it, meaning, it will duplicate the code for every data type used with the generics.
src/path/fs.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl Predicate<str> for BinaryFilePredicate { |
There was a problem hiding this comment.
Why is this implementation being added?
There was a problem hiding this comment.
@epage @kammitama5 and I had a look at this. You can't compare without this implementation. The example code will call Predicate<I: Item>::call(BinaryFilePredicate, I) so without this implementation you'll get the following errors.
error[E0277]: the trait bound `predicates::path::BinaryFilePredicate: predicates::Predicate<str>` is not satisfied
--> src/path/fs.rs:110:34
|
9 | assert_eq!(false, predicate_file.eval("src"));
| ^^^^ the trait `predicates::Predicate<str>` is not implemented for `predicates::path::BinaryFilePredicate`
|
= help: the following implementations were found:
<predicates::path::BinaryFilePredicate as predicates::Predicate<std::path::Path>>
<predicates::path::BinaryFilePredicate as predicates::Predicate<[u8]>>
There was a problem hiding this comment.
So this is being used to try to emulate AsRef<Path> with eval?
The original issue was only about eq_file. In thinking about the trade offs of adding this extra functionality, I'm leaning against it. Without this, the type system enforces what categories of information a predicate can deal with. This change removes that and makes it easy to use a eq_file predicate with any string predicate situation causing problems.
It might seem nice to users but I am expecting the main use case of predicates is that users create them and other libraries (like assert_cmd) consume them, so improving usability for this side doesn't offer as much benefit.
There was a problem hiding this comment.
@epage I don't see why that use case would justify a more unintuitive API, I think assumes the people writing the predicates have a pre-existing level of familiarity with predicates, which will never always be the case. Since these predicates are mostly going to be used in tests, most of the time the paths are going to be str literals anyway, why add the extra noise of Path::new("…") to their tests?
There was a problem hiding this comment.
As I said, my expectation is that the call to eval is not in the test but in a crate the test calls. In that case, string literals wouldn't be used and the crate author is probably dealing with a specific type anyways.
I also just checked the implementation. This conflicts with the intention of how the other predicate impls work:
impl Predicate<Path> for BinaryFilePredicate
impl Predicate<[u8]> for BinaryFilePredicate
impl Predicate<Path> for StrFilePredicate
impl Predicate<str> for StrFilePredicateThe non-path versions of the predicates allow using eq_file with in-memory test-values. Adding impl Predicate<str> for BinaryFilePredicate then has a different meaning then impl Predicate<[u8]> for BinaryFilePredicate and cannot be added to StrFilePredicate
There was a problem hiding this comment.
@epage I can agree that with that inconsistency would make it less than ideal, and I can't really find a better alternative.So I'm fine with that being removed.
There was a problem hiding this comment.
I will be sure to reference the issue and squash when given the ok. Thank you so much, both of you! :)
src/path/fs.rs
Outdated
| use std::path; | ||
|
|
||
| use std::path::PathBuf; | ||
| use std::path::Path; |
There was a problem hiding this comment.
While I know its common practice in Rust, I personally don't like useing concrete types.
The more interesting aspect is the inconsistency, there is both "use std::path" and using of concrete types. We should only do one or the other.
src/path/fs.rs
Outdated
| impl BinaryFilePredicate { | ||
| fn eval(&self, path: &path::Path) -> io::Result<bool> { | ||
| let content = read_file(path)?; | ||
| fn eval<P: AsRef<path::Path>>(&self, path: P) -> io::Result<bool> { |
There was a problem hiding this comment.
hmm..I guess not.
|
Looks good. Feel free to squash. |
to avoid Monomorphism bloating. Fixes Issue assert-rs#62 Use Predicate file path to compare strings in path Eq Predicate ensures equality in string path Change eq_file function to use Predicate directly Change tests to verify that function works Change read_file to use AsRef<Path> Correct fs::read to be compatible with compiler Change to read_file so not using fs::read remove .into() replace path:path with path Minimalize monomorphization bloat by using changed data type in one function Replace concrete types for consistency with rest of code base Does not require AsRef<Path> except in top-level function. This is for consistency with the rest of the codebase and minimalism of monomorphism bloat
3 participants
No description provided.