github / go-spdx Goto Github PK

👋 Thanks for the awesome implementation and Satisfies functionality.

Over in syft we're trying to upgrade how we handle the complex license statements from SPDX.

I noticed that parse.go and scan.go has a lot of really good private functionality for reading these complex SPDX expressions.

Would you all accept a PR that exposed a few methods of this package so we could incorporate the parsing logic into how we identify and parse license in our SBOM output?

There are currently some deprecated rules in the .golangci.yaml config that need to be removed

Description

As this package moves to an open source release, it is important that documentation reads well. The doc generator should be run and resulting documentation reviewed for clarity.

Including examples

While looking through documentation docs, I noticed some packages (e.g. json) have examples that are hidden until clicked (e.g. json Indent function). It would be great to use this for the spdxexp package, especially for the Satisfies method which has lots of examples.

Looking at the code for json's Indent function, it looks like the examples are defined in example_test.go. I don't know if this is only available to standard go packages or if the general documentation generator will process an example file in any package.

I would like to add unit tests for cmd/exceptions.go

In satisfies.go, the following code likely will mutate left...

func mergeLeftRight(left, right [][]*Node) [][]*Node {
	for _, r := range right {
		for j, l := range left {
			left[j] = append(l, r...)

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 that in the future the results of the parser may be re-used somewhere. This would be a hard bug to track down.

Description

If a license is invalid, the scan process is returning an error message showing only the first letter of the invalid license.

Expected

For a license named "BAD-LICENSE", the error message should be something like... "unknown license 'BAD-LICENSE' at offset 0".

Actual

For a license named "BAD-LICENSE", the error message is... "unexpected 'B' at offset 0".

Add documentation explaining major concepts and purpose for each file.

Add package level documentation that describes interconnections between major concepts in different files and how to use public API.

_NOTE: Use Go for identifying the go language. There is at least one use of GO in the code documentation.`

Description

An open parenthesis in an expression should always have a matching close parenthesis. In the initial implementation in PR #1 , it was noted that a mismatch was silently ignored and swallowed at least the next following token. This leads to an invalid set of tokens instead of an error message.

Expected Behavior

When a matching close parenthesis is not in an expression, an error message should be returned from the scan function.

Proposed New Test

Add the following test (and perhaps other variations) to test for unmatched parentheses to the TestScan function in scan_test.go.

		{"operator error - malformed parenthetical", "(MIT",
		    []token{}, errors.New("open parenthesis does not have a matching close parenthesis")},

Results with current code

expected: *errors.errorString(&errors.errorString{s:"open parenthesis does not have a matching close parenthesis"})
actual  : <nil>(<nil>)

expected: <nil>(<nil>)
actual  : []spdxexp.token{spdxexp.token{role:0x0, value:"("}, spdxexp.token{role:0x3, value:"MIT"}}

The original javascript converted incoming operators (e.g. AND) to lower case (e.g. and) during the tokenization process. The initial translation stayed as close to the original javascript as possible and reasonable with assumptions that a later process would require the output to be in the format created in the javascript.

I do not see the need for this case conversion in later processing. Recommending a refactor to not convert operators case.