-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Default values for readme
if not specified
#8277
Conversation
If the readme field is not specified, assume a default of "README.md" (temporarily). Further, if the readme field is set to "false", then this defaulting behavior is suppressed.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
@rfcbot fcp merge |
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
src/cargo/util/toml/mod.rs
Outdated
None => default_readme_from_package_root(package_root), | ||
Some(value) => match value { | ||
StringOrBool::Bool(false) => None, | ||
StringOrBool::Bool(true) => default_readme_from_package_root(package_root), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting case because readme = true
could actually still infer no actual README if the files are missing. Perhaps for now we could just make readme = true
an error? Otherwise we probably want to assert that the default readme is indeed present, and None
isn't returned in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in getting back to this.
What do you think of this approach: Similar to how the build
field is handled, if readme = true
, we assume a default value of README.md
(instead of possibly None
, as you pointed out). Then, we can validate that the file really exists while creating the ManifestMetadata
, and bail!
if the file doesn't exist. This would be more consistent with the behavior of the build
field, but I'm not sure how important that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds reasonable to me!
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. The RFC will be merged soon. |
⌛ Testing commit 58ee013 with merge ec1460a230ec7ef95fc1eea22b60e773c31cf8af... |
💥 Test timed out |
@bors: retry |
☀️ Test successful - checks-azure |
Update cargo 15 commits in 40ebd52206e25c7a576ee42c137cc06a745a167a..1ec223effbbbf9fddd3453cdcae3a96a967608eb 2020-06-01 22:35:00 +0000 to 2020-06-09 20:03:14 +0000 - Default values for `readme` if not specified (rust-lang/cargo#8277) - Fix tree completions. (rust-lang/cargo#8342) - Support `{prefix}` and `{lowerprefix}` markers in `config.json` `dl` key (rust-lang/cargo#8267) - Add environment variables to identify the binary and crate name (rust-lang/cargo#8270) - Bump to 0.47.0, update changelog (rust-lang/cargo#8336) - Nits: Remove unneeded mut and loop (rust-lang/cargo#8334) - 1.45 beta backports (rust-lang/cargo#8331) - Better error message when passing in relative path to Workspace::new (rust-lang/cargo#8321) - Don't hash executable filenames on apple platforms. (rust-lang/cargo#8329) - fix clippy warnings (rust-lang/cargo#8324) - Require latest libgit2 to pull in bugfixes (rust-lang/cargo#8320) - Fix an accidental raw access of field (rust-lang/cargo#8319) - Use mem::take to replace with Default values (rust-lang/cargo#8314) - Allow Windows dylibs without dll suffix. (rust-lang/cargo#8310) - Show alias in help message (rust-lang/cargo#8307)
Fix failure with missing readme. #8277 added implicit README support, but it also rejected parsing any manifest where the README was missing. This causes a problem because the README is often missing in many registry packages (for various reasons). This removes the validation at parsing time. Cargo has historically not had hard enforcement at the parsing stage. Whether or not the readme exists has always been enforced during publishing. I have added some extra context to the error message, and added a test to that effect. Fixes #8351
656: Remove readme field from Cargo.toml r=taiki-e a=taiki-e Since 1.46 (rust-lang/cargo#8277), cargo can automatically detect the value of the readme field. Co-authored-by: Taiki Endo <te316e89@gmail.com>
If the a value for
readme
is not specified in Cargo.toml, we will now check for the existence of files namedREADME.md
,README.txt
orREADME
. If one does exist, the name of that file will be defaulted in to the manifest for the project.This behavior can be suppressed if
readme
is set tofalse
.Closes #8133