Update _joinExpressions to preserve precedence #1087
Conversation
There was a problem hiding this comment.
Yes. This would be my first thought of fixing this as well. But looking at the test cases of stringify in service/test/lib/spdx.js, I have 2nd thought: the license expressions joined this way may not be the simplified form. stringify in the SPDX module has the ability to add and omit () based on precedence (e.g, MIT AND BSD-3-Clause WITH GCC-exception-3.1 OR CC-BY-4.0 AND Apache-2.0). Options to leverage this may include:
- avoid calling stringify the expression nodes in _createExpressionFromLicense, construct an AND expression node when joining all expression nodes, and finally use SPDX.stringify to serialize the AND expression node. This however means that the internals of expression node need to be used to construct AND node, which is not ideal
- Alternatively, in _joinExpressions, can add () to each license expression unconditionally, use AND to join them, and then return SPDX.normalize(joinedExpressionString). SPDX.normalize calls stringify internally, so the expression after normalization is the simplified form without unnecessary (). This approach is less efficient in that normalize parses the expression string again before stringify. But the advantage is that it is simple and delegates the knowledge of operators, precedence, and simplifying () to @clearlydefined/spdx.
Any thoughts?
|
As we discussed last week, I agree and would definitely prefer to let the library handle this logic. Actually, my preference would be to either figure out if |
|
@qtomlinson Do you think you might have time to carry this forward while I'm on vacation? I think that would be tremendously helpful! 🙏🏼 |
|
@lumaxis sounds good. The |
Update definition schema due to recent fixes: clearlydefined#1087 clearlydefined#1108
Update definition schema due to recent fixes: clearlydefined#1087 clearlydefined#1108
Fixes #1084