add processing the satisfies function #2
Conversation
refactors satisfies_test.go to share node definitions between tests
### Remaining Work Need to verify that all license and license ref options work. May need to add processing for some aspects of licenses and license refs. Some regression tests continue to fail. All are marked with a comment about the failure. Includes refactor to use *Node instead of string when processing satisfies.
elrayle
changed the title
|
One of the failing tests is |
spdxexp/satisfies.go
Outdated
| // firstNormalized := firstTree // normalizeGPLIdentifiers(firstTree) | ||
| // secondNormalized := secondTree // normalizeGPLIdentifiers(secondTree) | ||
| firstExpanded := firstTree.expand(true) | ||
| fmt.Println("firstExpanded: ", firstExpanded) |
There was a problem hiding this comment.
Should remove the Printlns before merging.
There was a problem hiding this comment.
Fixed by commit 8378803
spdxexp/satisfies.go
Outdated
| // OR Expression: "MIT OR Apache-2.0" becomes [["MIT"], ["Apache-2.0"]] | ||
| func (node *Node) expandOr() [][]*Node { | ||
| var result [][]*Node | ||
| if node.Left().IsLicense() { |
There was a problem hiding this comment.
Not sure if it's worth it, but it seems like this block and the matching block for node.Right() could become a function that just takes node.Left() or node.Right() as input, possibly with the result array passed in and returned.
There was a problem hiding this comment.
Fixed by commit 8378803
spdxexp/satisfies.go
Outdated
| func mergeLeftRight(left, right [][]*Node) [][]*Node { | ||
| for _, r := range right { | ||
| for j, l := range left { | ||
| left[j] = append(l, r...) |
There was a problem hiding this comment.
No change needed because of how this is used, but I just thought it worth mentioning that left might actually get mutated by this. append will add to the same slice pointer if there's space and only return a new one if it needs to allocate more memory.
There was a problem hiding this comment.
The process doesn't use left again, but it is probably cleaner to address this. Since left is created by the parser, it is conceivable in the future that the results of the parser may be re-used somewhere. This would be a hard bug to track down.
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed by commit 8378803
| SortLicenses(nodes) | ||
| prev := 1 | ||
| for curr := 1; curr < len(nodes); curr++ { | ||
| if *nodes[curr-1].LicenseString() != *nodes[curr].LicenseString() { |
There was a problem hiding this comment.
I'm surprised you need the pointer dereference here in *nodes[curr-1]
There was a problem hiding this comment.
LicenseString() returns *string, so the dereference is happening on that. If LicenseString() is called on an expression node, then LicenseString() returns nil.
There was a problem hiding this comment.
Ahh, thanks for explaining that!
| // Example: | ||
| // BEFORE {{"MIT", "GPL-2.0"}, {"ISC", "Apache-2.0"}} | ||
| // AFTER {{"Apache-2.0", "ISC"}, {"GPL-2.0", "MIT"}} | ||
| func deepSort(nodes2d [][]*Node) [][]*Node { |
There was a problem hiding this comment.
can expressions be arbitrarily nested? Does that cause any issue here? MIT AND (GPL-2.0 OR (WTFPL AND CC0))
There was a problem hiding this comment.
At this point, all expressions have been processed in a way that results in a 2-dimensional array of nodes. Your example becomes:
{{"MIT", "GPL-2.0"}, {"MIT", "WTFPL", "CC0"}}
Values in the inner arrays are ANDs. Each array set is ORed. This 2d array is interpreted as either "MIT AND GPL-2.0" is expected OR "MIT AND WTFPL AND CC0" is expected.
Following DRY principle, code for processing OR and AND terms were separated out to functions that can be used for the left and right terms.
Description
There are several processes for the satisfies function:
All processing for the satisfies function is complete except the remaining work described below.
Remaining Work
+operator and exceptions for licensesReviewer Notes
Reviewer Note: I am considering moving
*Nodereceiver functions tonode.go. They are defined in satisfies javascript code, so they were translated in place for expedience. There are several places that can benefit from a refactor.