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

Made camino an optional dependency. #194

Closed
wants to merge 1 commit into from

Conversation

morr0ne
Copy link
Contributor

@morr0ne morr0ne commented Jul 2, 2022

As I already said in #193 I don't feel like camino is necessary however there might people that relay/need this kind of behavior so I made it an optional dependency disabled by default. I also updated the changelog with the corresponding entry.

@taiki-e
Copy link
Contributor

taiki-e commented Jul 2, 2022

This does not seem right because the cargo feature should be additive.

For example, enabling this feature may break dependencies that do not enable this feature because some signatures of camino and std::path APIs are incompatible. Also, in some cases, third-party traits implemented in std::path are not implemented in camino, so the code that uses them will also be broken.

@morr0ne
Copy link
Contributor Author

morr0ne commented Jul 2, 2022

I assumed Utf8PathBuf implemented all the methods from PathBuf but I see how that can be a problem especially with third-party impls. Is there a better solution to make camino optional? Otherwise it might be better to remove camino entirely. As I said in #193 it adds needless restrictions considering that every Utf8PathBuf is a valid PathBuf but not the other way around.

@oli-obk
Copy link
Owner

oli-obk commented Jul 4, 2022

I assumed Utf8PathBuf implemented all the methods from PathBuf but I see how that can be a problem especially with third-party impls.

That's not the main issue, the main issue is that the types are different, so if someone passes that field to a function expecting a PathBuf, because their crate uses cargo_metadata without the camino feature, then that would break the moment any other crate in the deptree enables the feature.

Will comment on the issue

@morr0ne
Copy link
Contributor Author

morr0ne commented Jul 4, 2022

Yeah I see how that's a big very big issue. Maybe a better solution is a wrapper type kinda like the map in serde_json. But honestly I think camino should be removed all together.

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.

3 participants