-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix(toml): Convert warnings that licence
and readme
files do not exist into errors
#13921
base: master
Are you sure you want to change the base?
Conversation
493e0f4
to
5c35e29
Compare
@bors try |
fix(toml): Convert warnings that `licence` and `readme` files do not exist into errors ### What does this PR try to resolve? In this PR: - Changed the warning to a hard error and modified the associated test function; - Removed what should have been a redundant test function:`publish::publish_with_missing_readme`; - Since `cargo publish` is preceded by the execution of `cargo package`, the error message in the test `function bad_license_file` needs to be modified. issue: #13629 (comment). ### Additional information It seems that this is not enough, the current situation is that `cargo package` warns if `package.readme` is an empty string or the wrong file location, but if I cancel `package.readme`, no warning is generated. I'm wondering if I should judge `package.readme&licence` when executing `cargo package` and return an error if it doesn't exist? As this has not been done before, your advice is sought.
💔 Test failed - checks-actions |
@bors try |
fix(toml): Convert warnings that `licence` and `readme` files do not exist into errors ### What does this PR try to resolve? In this PR: - Changed the warning to a hard error and modified the associated test function; - Removed what should have been a redundant test function:`publish::publish_with_missing_readme`; - Since `cargo publish` is preceded by the execution of `cargo package`, the error message in the test `function bad_license_file` needs to be modified. issue: #13629 (comment). ### Additional information It seems that this is not enough, the current situation is that `cargo package` warns if `package.readme` is an empty string or the wrong file location, but if I cancel `package.readme`, no warning is generated. I'm wondering if I should judge `package.readme&licence` when executing `cargo package` and return an error if it doesn't exist? As this has not been done before, your advice is sought.
☀️ Try build successful - checks-actions |
license || license_file, | ||
documentation || homepage || repository | ||
); | ||
lacking!(description, documentation || homepage || repository); |
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.
Why did license || license_file
part get removed? It is still required to have license specified.
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.
I thought not specifying license || license_file
was supposed to be an error and not a warning?
let rel_msg = if path.is_absolute() { | ||
"".to_string() | ||
} else { | ||
format!(" (relative to `{}`)", pkg.root().display()) | ||
}; | ||
ws.gctx().shell().warn(&format!( | ||
|
||
let msg = format!( |
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.
Cargo checks only two fields, so it is fine just returning an Err
here . That would make the code easier to reason.
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.
It would certainly be possible to return Err
directly in the function error_on_nonexistent_file
, but I think if neither licence
nor readme
exists, then they should all be prompted at the same time, not one by one.
And it's also easier to expand the other options later on as required fields as well. I hope my understanding is reasonable, thank you.
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.
Hard error, even for empty string
Should we add test checking empty string like license-file = ""
or readme = ""
will fail?
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, it seems that there is already a corresponding test in the test function.
What does this PR try to resolve?
In this PR:
publish::publish_with_missing_readme
;cargo publish
is preceded by the execution ofcargo package
, the error message in the testfunction bad_license_file
needs to be modified.issue: #13629 (comment).
Additional information
It seems that this is not enough, the current situation is that
cargo package
warns ifpackage.readme
is an empty string or the wrong file location, but if I cancelpackage.readme
, no warning is generated.I'm wondering if I should judge
package.readme&licence
when executingcargo package
and return an error if it doesn't exist?As this has not been done before, your advice is sought.