initial implementation of scan, parse, and satisfies #1
Conversation
includes start of comparison methods needed for Satisfy
* start of satisfies translation from js to go * add comparator code to determine >, <, = for two licenses * add ranges for licenses * normalize -or-later, -only, + in license identifier Known direct license comparisons that fail in go, but pass in js ``` satisfies(“Apache-3.0”, “Apache-2.0+”) // Apache-3.0 doesn’t exist satisfies(“Apache-1.0+”, “Apache-2.0+”) // not in same range satisfies(“GPL-2.0”, “GPL-2.0+”) // not in same range satisfies(“GPL-2.0”, “GPL-2.0-or-later”) // not in same range ```
There was a problem hiding this comment.
Looks good, and definitely looks like some tricky porting from JavaScript!
There's no need to make a change like this, but I thought I'd mention it in case it makes Satisfies clearer/easier to implement. This is fresh in my mind having just worked on validation code that has to work across types… What I'm about to say only makes sense if Satisfies only takes a particular license or GPL-2.0-or-later kind of thing and not full expressions.
If you wanted Node to be something polymorphic, you could think in terms of what operations need to be performed. Satisfies is the big one, but maybe there are others that could be performed (greater than, less than). You then make an interface (e.g. Node) that's like Satisifes(string) bool. Then you can have a License struct that implements that interface and contains info about a single license (or a license range, whatever makes sense for that). Then you can have an And struct that only passes Satisfies if the two Nodes it contains pass Satisfies.
Not sure if that makes any sense or seems helpful at all.
|
|
||
| func (t *tokenStream) parseAtom() *Node { | ||
| parenNode := t.parseParenthesizedExpression() | ||
| if t.err != nil { |
There was a problem hiding this comment.
I'm reading this and the previous function as (MIT (no closing paren) will just silently disappear from the expression rather than bubbling up an error. Is that right? It seems like that could cause trouble.
There was a problem hiding this comment.
I'll add a follow-on PR to add a test.
There was a problem hiding this comment.
| // Advances the index if the operator is found. | ||
| func (t *tokenStream) parseOperator(operator string) *string { | ||
| token := t.peek() | ||
| if token.role == OPERATOR_TOKEN && token.value == operator { |
There was a problem hiding this comment.
Are these operators case sensitive? Is MIT or GPL-2.0 allowed?
There was a problem hiding this comment.
The ABNF in the spec defines them as all cap. The javascript parser expects all caps. In the javascript, the results of parsing converted them to all lower case. Not sure why they chose to do that. I did the same, expecting that as I moved farther into the code, it would become clear why this was done. But now that a significant junk of the satisfies code is done, I don't see a need to convert them. For now, I will leave the conversion to lower case in, but this would be a very simple refactor to remove an unnecessary conversion.
There was a problem hiding this comment.
| @@ -0,0 +1,293 @@ | |||
| package spdxexp | |||
|
|
|||
| /* Translation to GO from javascript code: https://github.com/clearlydefined/spdx-expression-parse.js/blob/master/scan.js */ | |||
There was a problem hiding this comment.
Nit: The O in Go is lower case 😃
There was a problem hiding this comment.
I'm going to give each Go file better documentation. I'll update this when I do that.
There was a problem hiding this comment.
| {role: LICENSE_TOKEN, value: "Apache-2.0"}, | ||
| {role: OPERATOR_TOKEN, value: ")"}, | ||
| }, nil}, | ||
| {"kitchen sink", " (MIT AND Apache-1.0+) OR DocumentRef-spdx-tool-1.2:LicenseRef-MIT-Style-2 OR (GPL-2.0 WITH Bison-exception-2.2)", |
|
Thanks for the suggestions about another approach for the structuring of nodes. Once the translation is complete, I can take a deeper look at potential refactors that can simplify the code and make it more closely follow typical Go code. |
Major Components
NOTE: Currently hardcoded. May want to get from external canonical source in the future to avoid longterm maintenance to keep the lists up to date.
Remaining Work
Satisfies needs to be extended to support simple conjunctive expressions (e.g.
"MIT AND Apache-2.0","MIT OR Apache-2.0", etc.) and complex deep tree expressions (e.g." (MIT AND Apache-1.0+) OR DocumentRef-spdx-tool-1.2:LicenseRef-MIT-Style-2 OR (GPL-2.0 WITH Bison-exception-2.2)").