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.

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

Accept an AST and a whitelist Array of AST leaves #11

Closed
wants to merge 3 commits into from

Conversation

kemitchell
Copy link
Member

@kemitchell kemitchell commented Mar 25, 2019

Taking off from: #7 (comment)

This PR redesigns the external API to take new kinds of arguments:

  1. the AST for an SPDX expression, in the form returned by spdx-expression-parse

  2. an Array of AST leaves, each corresponding to a license, license with exception, license range, or license range with exception

A few benefits:

  • This approach massively simplifies the underlying algorithm. We're back to straightforward recursion.

  • This approach leaves room for nonstandard license identifiers. spdx-expression-parse is now a devDependency, for the test suite. But neither the parser nor the license-list data package it depends on remain production dependencies.

  • Completely valid and completely unanswerable questions about the intended semantics are entirely avoided. Which is a good sign.

@kemitchell
Copy link
Member Author

Paging @jeffmcaffer.

@jeffmcaffer
Copy link
Contributor

@kemitchell this looks much simpler and has lots of great attributes. Love that the recursion logic follows exactly the same logic as the expression itself. The understanding that in the end we are just matching against a set of licenses rather than another expression that enables this.

Would love to stew on this for a day (or get @dabutvin so stew) to see if there are any gotchas.

@kemitchell
Copy link
Member Author

@jeffmcaffer I sent you an invite to the org. Hack responsibly!

@jeffmcaffer
Copy link
Contributor

Hah! Achievement unlocked!

@kemitchell
Copy link
Member Author

@jeffmcaffer any objection to my going ahead and releasing as semver-major? Alternatively, I’d you have strong reservations, I can fork and rename to meet my own immediate needs, and we can discuss reconciling when you have time.

@kemitchell
Copy link
Member Author

Closing this in deference to #17.

@kemitchell kemitchell closed this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants