Add PathBuf Arbitrary impl #362
Conversation
|
(author of camino here, including the I think a useful impl should have a certain number of components separated by https://docs.rs/camino/1.1.6/src/camino/proptest_impls.rs.html#14-42 |
|
Sorry for the delay in getting to this -- works been busy this week. I'll try to give this a look tonight. |
|
Awesome @sunshowers, that's the kind of the feedback I was hoping for. "The best way to get something done right is to do it wrong on the internet". :) |
| use std::path::*; | ||
| use std::{ffi::OsString, path::*}; | ||
|
|
||
| use crate::{arbitrary::StrategyFor, prelude::Strategy, strategy::MapInto}; | ||
|
|
||
| // TODO: Figure out PathBuf and then Box/Rc/Box<Path>. |
There was a problem hiding this comment.
we should edit/remove this comment once we get to a spot we're happy with
| arbitrary!(PathBuf, MapInto<StrategyFor<OsString>, Self>; | ||
| OsString::arbitrary().prop_map_into() | ||
| ); |
There was a problem hiding this comment.
agreed with @sunshowers comment that we should be a bit more robust in the impl here. I agree that a good starting place is to ensure we get a spread of path segments and we get coverage of absolute and relative paths. I'd be fine with merging a strategy that hits those two marks. I think more robust edge case generation could be done separately.
let me know if you'd be willing to tackle those in a follow up
There was a problem hiding this comment.
Unfortunately I won't have bandwidth to take this on probably for quite a while, but I hope this can be a very minimal bit of inspiration for others to build upon though.
Also I might point out that even though this is not an ideal implementation in terms of covering a wide range of possibilities, it does produce an arbitrary PathBuf, so maybe in the spirit of "something is better than nothing", this could be merged with the intention to improve upon it. Maybe even with a deprecation to make it clear that this is not a good implementation, but is currently the best we have? That would get me unstuck on my own project, where I really just need a completely arbitrary PathBuf, and have some types that contain PathBufs.
There was a problem hiding this comment.
Could either of you copy over the impl in camino? Otherwise I can try getting a PR up with that impl when I have time.
There was a problem hiding this comment.
Ended up doing this myself, see #368.
There was a problem hiding this comment.
Thanks! I'm on vacation until next week so may not get a chance to look at it but will try to look sometime next week if I don't find any time before then
Followup to proptest-rs#362. Includes documentation. Thanks to Michael for starting work on this! Co-authored-by: Michael Dougherty <maackle.d@gmail.com>
* Add PathBuf Arbitrary impl Followup to #362. Includes documentation. Thanks to Michael for starting work on this! Co-authored-by: Michael Dougherty <maackle.d@gmail.com> * Use PathParams --------- Co-authored-by: Michael Dougherty <maackle.d@gmail.com>
A minimal implementation of Arbitrary for PathBuf, per #272