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

Relaxed parsing? #11

Closed
danielmoore opened this issue Nov 5, 2016 · 5 comments · May be fixed by #16
Closed

Relaxed parsing? #11

danielmoore opened this issue Nov 5, 2016 · 5 comments · May be fixed by #16

Comments

@danielmoore
Copy link

The structure of the ABNF documented in Appendix IV of SPDX 2.1 makes it impossible to recover from an expression like (MIT OR BSD) using a tool like spdx-correct. Would it be reasonable to assume that all license-id tokens are a subset of all valid id-string tokens so we could apply validation optionally at the parse phase instead of the lex phase?

That is, could we do something like this:

var parse = require('spdx-expression-parse');
var assert = require('assert');

assert.deepEqual(
  parse('(MIT OR BSD)', { relax: true }),
  {
    left: { license: 'MIT' },
    conjunction: 'or',
    right: { license: 'BSD' }
  }
);
@danielmoore
Copy link
Author

bump

@kemitchell
Copy link
Member

Thanks for this PR. I'm not sure why I didn't see a notification the first time. I'm not usually a fan of bumps, but I'm thankful for this one!

It's certainly possible to write a parser that works that way. Just drop an identifier-like pattern in for the static lists of standardized IDs and exception IDs in the grammar. There isn't really much to the syntax after that. Parentheses, AND, OR, WITH, and +, IIRC.

However, I don't think that's what this package should be. Certainly for npm upstream, the main concern is single, invalid identifiers like BSD or Apache 2.0. Typos. Folks who aren't aware of the standard list. Gently nudging folks toward a cleaner package ecosystem.

Just FYI, non-list identifiers have certainly come up before. npm considered various "escape hatches" for custom and non-SPDX licenses, settling eventually on SEE LICENSE IN ____ as a valid license pattern package.json, completely distinct from SPDX. A few users chimed in for something like (MiT OR OTHER), with OTHER indicating the other prong of a dual-license offering. Spec-style LicenseRef-____ was also considered. All of these were issues on npm/npm, if you care to dig them up.

This parser actually retains support for LicenseRef and DocumentRef. Wrapper code between this package and npm CLI flags valid SPDX expressions with LicenseRefs as invalid package.json license properties.

@danielmoore
Copy link
Author

Gently nudging folks toward a cleaner package ecosystem.

I definitely agree with that sentiment and my request is in service to that. More specifically, I'd like to be able to show users where their license expression is incorrect and suggest how to fix it. You might think of this as spdx-correct on steroids:

$ validate-spdx "(MIT OR BSD)"
Error found: invalid license "BSD". Did you mean "BSD-2-Clause"?
(MIT OR BSD)
        ^^^

But I can't do that without an AST, and I can't get an AST if the license token lexemes are literally the license list. I suppose I could write my own library, but that would seem like poor reuse. At the same time, I think the default behavior should be as-is, where invalid licenses raise errors.

@kemitchell
Copy link
Member

kemitchell commented May 26, 2017

I suppose I could write my own library, but that would seem like poor reuse.

You're not going to bother me writing any new code, or even taking what I've written and turning it into your own package.

@kemitchell
Copy link
Member

@danielmoore, you may be interested in jslicense/spdx-correct.js#19, and especially @motet-a's work in #16.

I'd also like to hear about your use case, if you can share. I'm seeing more complex expressions in issues than I expected to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants