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

Allow bool for readme in manifest #235

Closed
wants to merge 1 commit into from
Closed

Allow bool for readme in manifest #235

wants to merge 1 commit into from

Conversation

heaths
Copy link

@heaths heaths commented May 30, 2023

Fixes #234

@ehuss
Copy link
Contributor

ehuss commented May 30, 2023

I'm a bit confused about this change. Cargo doesn't expose the true/false status as a boolean via metadata. It should only ever be a string or null. Can you say more about what exact scenario this is trying to address?

@heaths
Copy link
Author

heaths commented May 30, 2023

A manifest can explicitly declare it has no readme, as may be the case when using wasm-pack, for example. See rustwasm/wasm-pack#1215 for details, but effectively readme = false is allowed per documentation, which is intended to disable readme resolution. For example, even if you had a README.md but didn't want it to show in your crate description, you could set readme = false. Since wasm-pack took a dependency on this crate, I wanted to fix it upstream so that we could detect if the readme was explicitly elided.

@ehuss
Copy link
Contributor

ehuss commented May 30, 2023

Sorry, maybe I wasn't clear. The true/false value of the readme field isn't exposed by cargo (see cargo metadata). This crate is just representing the values that cargo itself exposes via the cargo metadata command. A value of readme=false is translated to null, and a value of true is translated to "README.md".

@heaths
Copy link
Author

heaths commented May 30, 2023

Ah. Thanks for clarifying. I looked through the project de/serialization a bit but didn't realize you were effectively parsing the output of a command. Given the "translation" you mentioned, that should still be enough to go on to determine if the readme resolution was disabled, though.

Thank you.

@jplatte
Copy link

jplatte commented May 30, 2023

Well, if the cargo metadata output indeed makes a difference here (null vs. an absent field, or is it never absent?), I think it would be reasonable for this crate to reflect that. I wrote the js_option crate for exactly this use case of distinguishing null vs. missing, though it might not be desireable to take on the extra public dependency here.

@ehuss
Copy link
Contributor

ehuss commented May 30, 2023

cargo metadata does not distinguish a difference (the field is always present).

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.

readme can also be a boolean
3 participants