-
Notifications
You must be signed in to change notification settings - Fork 323
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
Clearer warnings in license review #9134
Clearer warnings in license review #9134
Conversation
…tal (as they were breaking anyway)
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.
Thanks for the improvements. The error messages as well as few style changes look neat.
…better-warnings-in-license-review
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.
(approving Rust parts)
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.
Nit picks regarding the naming. I find the notice
and problem
a bit ambiguous. warning
and error
are less confusing.
s"A license file was discovered in $name that is different " + | ||
s"from the default license file that is associated with its " + | ||
s"license ${dep.information.license.name}, but a custom license was not expected. If this custom license should override the default one, create a `custom-license` config file. If both files are expected to be included, create an empty `default-and-custom-license` file." |
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.
s"A license file was discovered in $name that is different " + | |
s"from the default license file that is associated with its " + | |
s"license ${dep.information.license.name}, but a custom license was not expected. If this custom license should override the default one, create a `custom-license` config file. If both files are expected to be included, create an empty `default-and-custom-license` file." | |
s"""|A license file was discovered in $name that is different | |
|from the default license file that is associated with its | |
|license ${dep.information.license.name}, but a custom license was not expected. If this custom license should override the default one, create a `custom-license` config file. If both files are expected to be included, create an empty `default-and-custom-license` file.""".stripMargin |
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 will add newlines into the string, whereas before the string was a single line.
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 don't feel as a suitable reviewer.
s"No files or copyright information are included for $name." | ||
diagnostics.append( | ||
Diagnostic.Problem( | ||
s"No files or copyright information are included for $name. Generally every dependency should have _some_ copyright info, so this suggests all our heuristics failed. Please find the information manually and add it using `files-add` or `copyright-add`. Even if the dependency is public domain, it may be good to include some information about its source." |
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.
+1
Thanks @hubertp for the suggestions, I've applied them. |
Pull Request Description
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.