-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add method for extracting licenses from expression #52
Conversation
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.
Looks good overall! I just wonder about the possibility of a panic. It's good that there are tests that show this working in a variety of cases.
spdxexp/satisfies_test.go
Outdated
{"AND'ed licenses", "MIT AND Apache-2.0", []string{"MIT", "Apache-2.0"}}, | ||
{"AND'ed & OR'ed licenses", "(MIT AND Apache-2.0) OR GPL-3.0", []string{"GPL-3.0", "MIT", "Apache-2.0"}}, | ||
{"ONLY modifiers", "LGPL-2.1-only OR MIT OR BSD-3-Clause", []string{"MIT", "BSD-3-Clause", "LGPL-2.1-only"}}, | ||
{"WITH modifiers", "GPL-2.0-or-later WITH Bison-exception-2.2", []string{"GPL-2.0-or-later"}}, |
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.
Huh, that brings up an interesting question about whether exceptions should be preserved in some fashion. It probably doesn't make a huge difference in what we need this for now, but it is relevant in general to know that a piece of code is licensed with exceptions.
I wonder if it's worth considering the license to be GPL-2.0-or-later WITH Bison-exception-2.2
instead of just the GPL part, but I'm also wary of complicating what we're working on. Probably something to think about in a followup.
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'm wondering if reconstructedLicenseString()
in node.go
has a role in this. The current approach appears to use the simple license ignoring LicenseRed, DocumentRef, and Exceptions. That takes a node and returns the license string with license ref, document ref, and exceptions put back in the string.
case
{"WITH modifiers", "GPL-2.0-or-later WITH Bison-exception-2.2", []string{"GPL-2.0-or-later"}},
would become
{"WITH modifiers", "GPL-2.0-or-later WITH Bison-exception-2.2", []string{"GPL-2.0-or-later WITH Bison-exception-2.2"}},
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 think this is a better way to go. It's more accurate to think of the license+exceptions as a different license.
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.
Updated it to include modifiers like above
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.
Since extract isn't used by the satisfies process, it would make more since to me that it lives in extracts.go. I like the new helpers.go. A refactor might pull out other functions (if any) that are used by both satisfies() and extract().
spdxexp/satisfies_test.go
Outdated
{"AND'ed licenses", "MIT AND Apache-2.0", []string{"MIT", "Apache-2.0"}}, | ||
{"AND'ed & OR'ed licenses", "(MIT AND Apache-2.0) OR GPL-3.0", []string{"GPL-3.0", "MIT", "Apache-2.0"}}, | ||
{"ONLY modifiers", "LGPL-2.1-only OR MIT OR BSD-3-Clause", []string{"MIT", "BSD-3-Clause", "LGPL-2.1-only"}}, | ||
{"WITH modifiers", "GPL-2.0-or-later WITH Bison-exception-2.2", []string{"GPL-2.0-or-later"}}, |
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'm wondering if reconstructedLicenseString()
in node.go
has a role in this. The current approach appears to use the simple license ignoring LicenseRed, DocumentRef, and Exceptions. That takes a node and returns the license string with license ref, document ref, and exceptions put back in the string.
case
{"WITH modifiers", "GPL-2.0-or-later WITH Bison-exception-2.2", []string{"GPL-2.0-or-later"}},
would become
{"WITH modifiers", "GPL-2.0-or-later WITH Bison-exception-2.2", []string{"GPL-2.0-or-later WITH Bison-exception-2.2"}},
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.
Looks great with the changes! Just one suggestion to remove that new nil
check.
Looks good to me! |
This adds a new method
ExtractLicenses
which pulls all of the licenses out of an expression string (without duplicates)I added this into the
Satisfies
file where the rest of the public methods are but if there's a better spot let me know 🤓