Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
LicenseChoices: Add repository license choices #3801
LicenseChoices: Add repository license choices #3801
Changes from all commits
69ff88e
eafe2ef
779ceda
dd0b2ae
db9cfa5
2577355
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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: The extraction of this function could have happened in a preparing commit, as it's unrelated to the actual change.
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 would prefer to leave these changes together. As with the extraction came a fix for the actual test we weren't aware of before.
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 do
repositoryLicenseChoices
require agiven
butpackageLicenseChoices
do not?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.
When using a package license choice we have a specific effective license of the package a choice (or several) can be applied to, which is most likely to be known before applying the choice. When we have a repository license choice, the license choice would be applied to each package that offers this license as a choice. Making sure that there is a given helps only applying the choice to a wanted given as opposed to all licenses with that choice, which could lead to unwanted choices.
For consistency it would be possible to make
given
mandatory forpackageLicenseChoices
, but this was previously discussed before and decided otherwise.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 explanation, can you add that to the code docs, e.g. for the
LicenseChoices
class or the properties? And maybe also to the documentation from the last commit?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 added the explanation to the
RepositoryLicenseChoices
, but am unsure if this is how and where it belongs.I also added it to the documentation.
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, I think this is good enough for now. I just wanted to avoid that when you read this code without knowing the context you stumble over the fact that only one of the properties has this requirement.