Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add (optional, feature-gated) proptest::Arbitrary implementation #18

Merged
merged 1 commit into from
May 10, 2022

Conversation

mcronce
Copy link
Contributor

@mcronce mcronce commented Apr 15, 2022

This makes it easier to proptest code that makes use of camino

Copy link
Collaborator

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution! I love proptest and I'm happy to see camino gain support for it.

Cargo.toml Outdated
Comment on lines 27 to 32
proptest = { version = "1.0.0", optional = true }
serde = { version = "1", features = ["derive"], optional = true }

[features]
serde1 = ["serde"]
proptest = ["dep:proptest"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work with older versions of Cargo, which camino supports. I'd recommend calling the feature proptest1 to match serde1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines +14 to +28
impl Arbitrary for Utf8PathBuf {
type Parameters = <String as Arbitrary>::Parameters;
type Strategy = MapInto<StrategyFor<String>, Self>;
fn arbitrary_with(args: Self::Parameters) -> Self::Strategy {
any_with::<String>(args).prop_map_into()
}
}

impl Arbitrary for Box<Utf8Path> {
type Parameters = <Utf8PathBuf as Arbitrary>::Parameters;
type Strategy = MapInto<StrategyFor<Utf8PathBuf>, Self>;
fn arbitrary_with(args: Self::Parameters) -> Self::Strategy {
any_with::<Utf8PathBuf>(args).prop_map_into()
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like a very representative generation strategy since most strings won't have slashes in them -- seems like it should instead produce path components and then join them according to the platform's rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's something I would generally leave to individual library/application testers to provide, rather than assuming their data for them. It's also something that could be added on here in the future as a default if there's demand for it. These are essentially copies of the OsString and Box<OsStr> impls; anything further is outside my skillset as far as implementing Arbitrary.

@sunshowers
Copy link
Collaborator

Thanks -- I think I'm going to land this and then iterate on it locally.

@sunshowers sunshowers merged commit d2e7348 into camino-rs:main May 10, 2022
@sunshowers
Copy link
Collaborator

This is out as part of camino 1.0.8 and above (just released 1.0.9 with some documentation fixes).

@mcronce
Copy link
Contributor Author

mcronce commented May 24, 2022

Awesome. Thanks!

Just to provide a little extra flavor about my use case, I'm using proptest to validate parsing/reserializing SFTP packets in https://gitlab.cronce.io/foss/sftp-rs/-/tree/master/sftp-protocol/src/packet - technically any valid UTF-8 string is a valid path, as nonsensical as it might be (e.g. the string "LeɬXYVº5Þ=7Ԏ!ŚϔO" could very well be referring to a file named LeɬXYVº5Þ=7Ԏ!ŚϔO in the current working directory...so the parser needs to correctly handle that case)

In particular, paths in SFTP are specified to be UTF-8 unless a protocol extension says otherwise. I'm taking an opinionated stance and not supporting any protocol extensions of that nature :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants