add option to override parse function

[dabutvin]

No description provided.

[kemitchell]

What's your use case?

This package doesn't requiring injection of spdx-expression-parse because that's the only implementation that it was intended to work with.

Allowing users to override spdx-expression-parse doesn't save them having to install it as an indirect dependency.

[dabutvin]

Yep. It's to be able to configure spdx-expression-parse for things like the relaxed option or licenseVisitor that are in flight

[dabutvin]

rebased to resolve the conflicts.

@kemitchell any thoughts on landing this to allow options to flow to spdx-expression-parse?

My main goal is to allow the proposed licenseVisitor so that I can do satisifes like

satisfies('mit', 'MIT OR Apache-2.0')

[kemitchell]

If @motet-a is still available and interested in spdx-expression-parse, I'd like his input on the changes there. If we end up shipping -parse with an options argument, we should definitely consider publishing a new -satisfies that takes either a parser argument, or an options object to pass to -parse.

[kemitchell]

@dabutvin If you have pressing needs that require packages in the public registry, feel free to publish forks as new packages. We can always re-merge and deprecate.

[dabutvin]

👍 Makes sense to me.
I'll be patient and check back in a bit :) Thanks @kemitchell

[jeffmcaffer]

@kemitchell What's your thinking here? We are working out of a fork (OK) so this is not super critical but if would be great to just wrap up. This is a pretty modest change that would be still be relevant even if parse ends up having options.

[kemitchell]

@jeffmcaffer I'd support a new package that implements a parser for the SPDX grammar with invalid identifiers, and a patch to spdx-expression-parse that runs that new parser, then validates that all identifiers are valid SPDX identifiers. Would be happy to host the new, generic parser here in jslicense.

[jeffmcaffer]

@kemitchell my thinking here was that even with a more parser choices it would be good to allow satisfies to be configured with the parser to use (this PR) rather than an option to determine which parser to create. That is more future-proof and enables people to innovate on parsing as they see fit.

[kemitchell]

@jeffmcaffer not ignoring you. Just very, very busy.

[kemitchell]

@jeffmcaffer, by chance, is the root cause of your problem here a bottleneck getting new licenses onto the SPDX list?

[jeffmcaffer]

In a sense. IIRC, we need to handle expressions that have NOASSERTION as a valid element. We are not using LicenseRef to date but that would be a thing. In the case of this module it is really separating the concerns of parsing the expression and checking satisfaction. The other proposed changes to spdx-parse are certainly related

[dabutvin]

Another aspect to this is enabling flexibility within the parser, such as being case insensitive.

I think it's valuable to be able to have this work in certain situations

satisfies('mit', 'MIT OR Apache-2.0')

[kemitchell]

@dabutvin

satisfies(correct('mit'), '(MIT OR Apache-2.0)')

[kemitchell]

@jeffmcaffer I don't recall offhand. Is NOASSERTION a valid identifier in SPDX license expressions?

[jeffmcaffer]

@kemitchell spdx-correct is pretty aggressive and resulting in false corrections. In any event, the scenario remains, it would be great to have parsing separate from satisfaction (or any other) computations. An alternate design approach is to allow calling either with a string (in which case the default parse is used) or an already parsed structure.

So, for example, satisfies(parse('mit'), 'MIT') where parse is a whatever parsing tech the caller wants. That would be fine as well for our scenarios.

[kemitchell]

@jeffmcaffer: How would you feel about a major version that accepts an array of acceptable identifiers as rule and an AST in spdx-expression-parse shape?

[jeffmcaffer]

I did not answer @kemitchell's question about NOASSERTION. You are likely right but, most/all of the computations (e.g., satisfies) should not care, just parsing. the computations will just do string equality etc. Having NOASSERTION be in an expression is very important otherwise data is lost. For example, in trying to merge together the licenses from 3 files with license tool output of MIT, ISC, NOASSERTION, ideally we would get MIT AND ISC AND NOASSERTION. That helps readers know that there is something funky going on and they need to look deeper.

Of course, it would be great if we could always have a id for every license but given the 000s of licenses out there and the 00s of SPDX ids, that's not practical. Even with the ongoing thinking about dynamic license identification and registration we will likely still have NOASSERTION cases.

So, I would lobby to have NOASSERTION be a valid part of SPDX expressions.

[jeffmcaffer]

@jeffmcaffer: How would you feel about a major version that accepts an array of acceptable identifiers as rule and an AST in spdx-expression-parse shape?

That could be OK with some tweaks

• the array of ids should not run through a parse. If they are then we have the same issue as this PR is trying to avoid.
• The ids would have to all be normalized, canonicalized, ... satisfies could do this but in the current code at least, that requires the ids to actually be expression nodes.
• Given the potential complexity of SPDX expressions, we would need a helper function that took an expression object and returned the list of all licenses mentioned. Effectively flatten in the current code. Essentially we'd just be making callers do flatten(normalizeGPLIdentifiers(parse(second))) (see https://github.com/jslicense/spdx-satisfies.js/blob/master/index.js#L134).

Perhaps we can roll back to your basic issues with the proposed change. Might be that we are just not aligned on a fundamental API design philosophy or something. More than happy to try and fit into the approach but am fuzzy on how.

[kemitchell]

Might be that we are just not aligned on a fundamental API design philosophy or something.

Don't go there yet!

I'm mostly just really, really, really busy. But I have some time for this tonight.