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.

Sign up for GitHub

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

Jump to bottom

Make AnyLicenseInfo inherit from Element #369

Merged
merged 7 commits into from
Jun 13, 2023
Merged

Make AnyLicenseInfo inherit from Element #369

merged 7 commits into from
Jun 13, 2023

Conversation

goneall
Copy link
Member

@goneall goneall commented Jun 5, 2023

This pull request make just the changes necessary for LicenseField to inherit from Element.

This decouples this change from the other changes proposed in PR #297

@goneall
Make LicenseField inherit from Element
67ab6ab 
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall goneall requested review from swinslow and zvr June 5, 2023 23:12
@goneall
Copy link
Member Author

goneall commented Jun 5, 2023

After seeing the changes, I realized we are now requiring the expressions as well as licenses and exceptions now require a permanent ID. I think this is OK, it is just a change from SPDX 2.X where expressions could be anonymous but the licenses themselves have to have ID’s.

swinslow
swinslow reviewed Jun 6, 2023
View reviewed changes
Copy link
Member

@swinslow swinslow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @goneall, thanks for this! A few comments:

  1. For License and LicenseAddition, would we also want to drop seeAlso, and change to have license cross-references also be ExternalReferences?
  2. I think that License and LicenseAddition (not just LicenseField) would also need to be specified as a subclass of Element, if we're removing the Id, Name, etc. fields from LicenseAddition. Or I might be misunderstanding what the goal was here?

@goneall
Copy link
Member Author

goneall commented Jun 6, 2023

For License and LicenseAddition, would we also want to drop ``seeAlso and change to have license cross-references also be ExternalReferences

Good point - I'll remove seeAlso

I think that License and LicenseAddition (not just LicenseField) would also need to be specified as a subclass of Element

LicenseAddition and License both indirectly subclass from LicenseField which subclasses from Element, so they will both inherit the Element properties

@goneall
Remove seeAlso property from Addition and License
bfed668 
Per suggestion in pull request comments

Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall
Updates to license model from 6 June tech call
b68b895 
Changes NoneLicense and NoAssertionLicense to Individuals
Makes NoneLicense and NoAssertionLicense type AnyLicenseInfo
Removes LicenseField class
Add a new class ExtendableLicense to allow WithOperator to have
either a License or an OrLater operator

Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall
Copy link
Member Author

goneall commented Jun 6, 2023

@swinslow @zvr I updated this PR to match the decisions from today's call

zvr
zvr requested changes Jun 6, 2023
View reviewed changes
Copy link
Member

@zvr zvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typos and nitpicks

model/Licensing/Classes/ExtendableLicense.md Outdated Show resolved Hide resolved
model/Licensing/Classes/ExtendableLicense.md Outdated Show resolved Hide resolved
model/Licensing/Classes/LicenseAddition.md Show resolved Hide resolved
model/Licensing/Classes/AnyLicenseInfo.md Outdated Show resolved Hide resolved
@goneall
Corrections per review comments
26a737f 
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall
Copy link
Member Author

goneall commented Jun 7, 2023

Typos and nitpicks

Thanks @zvr - these should all now be fixed

zvr
zvr requested changes Jun 7, 2023
View reviewed changes
model/Licensing/Classes/AnyLicenseInfo.md Outdated Show resolved Hide resolved
model/Licensing/Individuals/NoAssertionLicense.md Outdated Show resolved Hide resolved
@goneall
Update AnyLicenseInfo text per suggestion
8deb424 
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall
Copy link
Member Author

goneall commented Jun 7, 2023

OK - this is a bit frustrating - I pushed a commit to correct the wording. It shows up in the branch commits but doesn't show up in the pull request commits.

@zvr - any ideas? I did to a force push yesterday, not sure if that messed up the pull requests. I guess I could close this and re-open another pull request with the same branch to fix, but we'd loose the conversation.

@goneall goneall changed the base branch from main to definingDocument June 7, 2023 17:37
@goneall goneall changed the base branch from definingDocument to main June 7, 2023 17:37
@goneall
Copy link
Member Author

goneall commented Jun 7, 2023

any ideas? I did to a force push yesterday, not sure if that messed up the pull requests. I guess I could close this and re-open another pull request with the same branch to fix, but we'd loose the conversation.

@zvr - Never mind - I found this workaround. Based on the high score on stack overflow, it looks like I'm not the only one who's run into this.

@goneall goneall changed the title Make LicenseField inherit from Element Make AnyLicenseInfo inherit from Element Jun 7, 2023
@goneall goneall mentioned this pull request Jun 9, 2023
Closed
@goneall
Update license related profiles per issue #372
dc3f731 
Adds LicenseExpression class
Creates a new profile for the complex license expression classes
Fixes #372

Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall
Copy link
Member Author

goneall commented Jun 9, 2023

@zvr @maxhbr @davaya @swinslow I updated the PR per the results of the 6 June meeting. Please review - a good chance I missed something ;)

@goneall
Fix parsing error
dcd11f2 
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
zvr
zvr approved these changes Jun 13, 2023
View reviewed changes
Copy link
Member

@zvr zvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good enough for merging; we might tweak texts later.

@goneall goneall merged commit c75ce38 into main Jun 13, 2023
1 check passed
@goneall goneall deleted the licelementsuper branch June 13, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swinslow swinslow swinslow left review comments

@armintaenzertng armintaenzertng armintaenzertng left review comments

@zvr zvr zvr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@goneall @zvr @swinslow @armintaenzertng