Extend test cases #25
Conversation
tests/validate_license_expr.rs
Outdated
| " MIT", | ||
| " MIT ", | ||
| " MIT ", | ||
| "MIT+", |
There was a problem hiding this comment.
While this is technically legal with the SPDX license expression syntax, I'd like to have future SPDX versions be able to error when + is applied to a license for which it makes no sense. I don't think the SPDX License List has any non-deprecated identifiers where + makes sense, and GPL-3.0+ and similar are deprecated short identifiers even without a + operator. So for testing +, I'd prefer using AGPL-1.0+ (and AGPL-1.0 + where you have MIT + below). The AGPL-1.0 identifier is deprecated in 3.1, but it exercises the + operator in a way that makes legal sense.
There was a problem hiding this comment.
I'm going to have to read a bit more around this, and see what other libraries are doing. I agree that MIT+ doesn't make sense, but as you say it's technically legal.
I'm thinking about how to add warnings to the API as well, for things like deprecated identifiers, so adding a + onto a license where it doesn't make sense could be a warning until the spec explicitly denies it.
There was a problem hiding this comment.
I'm thinking about how to add warnings to the API as well, for things like deprecated identifiers, so adding a + onto a license where it doesn't make sense could be a warning until the spec explicitly denies it.
That would be great (links to similar comments from here).
On the MIT+ vs. AGPL-1.0+ front, have you settled on a preference? I prefer AGPL-1.0+ because I weight "makes legal sense" more than "based on a non-deprecated identifier". But I don't feel stronlgy enough about it to hold up this PR, which looks good to me otherwise.
There was a problem hiding this comment.
Changed the tests to use AGPL-1.0+
| "WITH", | ||
| "MIT OR WITH", | ||
| // "MIT WITH", // TODO: Incorrectly marked as valid | ||
| // "MIT AND", // TODO: Incorrectly marked as valid |
There was a problem hiding this comment.
Is there a Rust analog to TAP's TODO tests for marking expected test failures? It would be nice to report these known failures as non-fatal warnings in the test results and be prompted to remove the TODO once we fix the backing implementation.
There was a problem hiding this comment.
Not in the default test runner that I know of. I'm hopefully going to fix these errors soon when I work on parsing to an actual syntax tree, which will also involve adding a whole bunch more test cases, so I think keeping them as just comments for now should be ok.
| "MIT Apache-2.0", | ||
| "MIT and Apache-2.0", | ||
| "MIT or Apache-2.0", | ||
| "GPL-3.0+ with Classpath-exception-2.0", |
There was a problem hiding this comment.
There's some discussion about making operators case insensitive in spdx/spdx-spec#63, but the 2.1 ABNF requires uppercase operators. So I'm fine requiring failures here for now, but we may need to revisit these if the SPDX discussion results in them (or license expressions as a whole) becoming case insensitive.
There was a problem hiding this comment.
Sounds good, I presume that can be done as a non-breaking enhancement to the spec so the same can happen here.
Changes since v1.3.0: * Updated SPDX License List to 3.1 (ehuss#9, ehuss#11, ehuss#17, ehuss#19, ehuss#24) * Updated SPDX License Expression reference from 2.0 to 2.1 (ehuss#8). * Document our license-list version, parens issue, and license (ehuss#16, ehuss#17). * Add Travis CI configuration (ehuss#22). * Add additional test cases (ehuss#25). * .mailmap: Consolidate authors (ehuss#26).
Changes since v1.3.0: * Updated SPDX License List to 3.1 (ehuss#9, ehuss#11, ehuss#17, ehuss#19, ehuss#24). * Updated SPDX License Expression reference from 2.0 to 2.1 (ehuss#8). * Document our license-list version, parens issue, and license (ehuss#16, ehuss#17). * Add Travis CI configuration (ehuss#22). * Add additional test cases (ehuss#25). * .mailmap: Consolidate authors (ehuss#26).
No description provided.