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

Change build field to Option<StringOrBool> #18

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

Turbo87
Copy link
Collaborator

@Turbo87 Turbo87 commented Aug 4, 2023

https://doc.rust-lang.org/cargo/reference/manifest.html#the-build-field says that valid values for the build field are paths and false, which basically matches the readme field (except for the workspace inheritance). Since readme uses the StringOrBool enum, it makes sense to use the same for the build field as well.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Turbo87
Copy link
Collaborator Author

Turbo87 commented Aug 4, 2023

@LukeMathWalker since StringOrBool is exclusively used for paths, any thoughts on changing it to PathBuf instead of String?

@LukeMathWalker
Copy link
Owner

Can you have a string field in a TOML document that is a valid PathBuf but not a valid String? @Turbo87

@Turbo87
Copy link
Collaborator Author

Turbo87 commented Aug 4, 2023

Can you have a string field in a TOML document that is a valid PathBuf but not a valid String?

TOML is defined as only allowing UTF8 and the thing that makes conversion between String and PathBuf tricky is non-UTF8 characters. In other words, a valid TOML file should already ensure that String can be converted to PathBuf and vice-versa without issues.

@LukeMathWalker
Copy link
Owner

OK, that's what I remembered. Then I think moving to PathBuf would be a step backwards, since we no longer signal in the type system the guarantee that the value is valid UTF8.

A potential alternative would be to use something like Utf8PathBuf from camino to give us the best of both worlds: precise modelling of the guarantees and path-related methods and conversions.

@Turbo87
Copy link
Collaborator Author

Turbo87 commented Aug 5, 2023

Since Path::new(String) is essentially free I guess it's indeed fine to keep it as a String. I'm wondering about adding something like fn as_path(&self) -> Option<&Path> to StringOrBool though (Option because of the Bool variant).

@LukeMathWalker
Copy link
Owner

I'd rather have a specialized PathBufOrBool, since an as_path method on StringOrBool would be a bit weird.
I'll merge this one in the meantime.

@LukeMathWalker
Copy link
Owner

Reviewing the docs, I'm not sure we should use StringOrBool here, but rather StringOrFalse, since true isn't a valid value for the build field.

@Turbo87
Copy link
Collaborator Author

Turbo87 commented Aug 6, 2023

https://github.com/rust-lang/cargo/blob/b644a40a1f8406a08bc64305a6a8fd6bb5a34071/src/cargo/util/toml/mod.rs#L1510 suggests that this is mostly a documentation issue. apparently it behaves similar to https://github.com/rust-lang/cargo/blob/b644a40a1f8406a08bc64305a6a8fd6bb5a34071/src/cargo/util/toml/mod.rs#L1532, except for the inheritance.

@LukeMathWalker LukeMathWalker merged commit 7e91d45 into LukeMathWalker:master Aug 15, 2023
3 of 4 checks passed
@Turbo87 Turbo87 deleted the build-type branch August 30, 2023 15:07
@Turbo87 Turbo87 added the enhancement New feature or request label Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants