Add PathBuf Arbitrary impl with tests #368
Conversation
Followup to proptest-rs#362. Includes documentation. Thanks to Michael for starting work on this! Co-authored-by: Michael Dougherty <maackle.d@gmail.com>
|
For me a number of tests failed, I'm not sure why: https://gist.github.com/sunshowers/5ad72397fe70205bbb27cae534d65443#file-gistfile0-txt-L4551-L4685 But those tests also failed for me on main. (This is with |
|
I have a feeling this isn't quite correct. I think it might generate paths that are too long. Going to try looking at it later this week. |
proptest/src/arbitrary/_std/path.rs
Outdated
|
|
||
| fn arbitrary_with(args: Self::Parameters) -> Self::Strategy { | ||
| static_map( | ||
| (any::<bool>(), any_with::<Vec<OsString>>(args)), |
There was a problem hiding this comment.
i'm not familiar with the OsString arbitrary impl but I'm wondering if it can generate a string with a delimiter in it. will pull this locally and mess around more. otherwise this looks good.
There was a problem hiding this comment.
OsString strategy does indeed generate strings with the delimiter.
There was a problem hiding this comment.
I think that's okay, the number of delimiters is not a hard and fast rule here.
I'm more concerned about the number of components being too large by default.
I'm wondering if we should define a PathParameters struct with a custom default value for the number of delimiters.
There was a problem hiding this comment.
I did notice that quite a few components were getting generated. I think a PathParameters struct with a default number and ensuring the output has that number (which may involve trimming any excess generated) could make sense. I think it'd be an un-intuitive if a user asks for n segments and we give them n + m.
There was a problem hiding this comment.
That makes sense. You're welcome to work on it, or I'll get around to it at some point later this week (speaking at RustConf in 3 days 😅)
There was a problem hiding this comment.
@matthew-russo OK, done. Let me know if this makes sense.
There was a problem hiding this comment.
Sorry work and personal life have kept me quite busy so I missed your last message. Thanks for getting back to this and congrats on the talk!
sunshowers
force-pushed
the
arbitrary-pathbuf
branch
from
cfac129 to
839ba52
Compare
|
|
||
| arbitrary!(StripPrefixError; Path::new("").strip_prefix("a").unwrap_err()); | ||
|
|
||
| /// A private type (not actually pub) representing the output of [`PathParams`] that can't be |
There was a problem hiding this comment.
non-blocking: it is actually pub(?) not sure if this just means it isn't intended to be publicly used or if this is residual from previous iterations.
There was a problem hiding this comment.
I meant that even though it is pub, it is not exported as an API so that backwards compatibility is maintained. Any suggestions for better phrasing?
There was a problem hiding this comment.
I don't think its a huge deal either way. I would probably just say something like:
"An opaque type representing the output of ...
Followup to #362. Includes documentation. I'm pretty new to this and I had to cargo-cult a bit so please point out issues!
Thanks to Michael for starting work on this!