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

allow and, or and with operators #876

Closed
wants to merge 1 commit into from

Conversation

xsuchy
Copy link

@xsuchy xsuchy commented Feb 4, 2024

This is follow up of discussion https://lists.spdx.org/g/spdx/message/1798 where this was found as good compromise https://lists.spdx.org/g/spdx/message/1817

This is follow up of discussion https://lists.spdx.org/g/spdx/message/1798
where this was found as good compromise https://lists.spdx.org/g/spdx/message/1817

Signed-off-by: Miroslav Suchý <[email protected]>
@zvr
Copy link
Member

zvr commented Feb 4, 2024

I assume this should be oriented towards the development/v3.0 branch.

Copy link
Member

@maxhbr maxhbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set of changes looks good

@maxhbr
Copy link
Member

maxhbr commented Feb 5, 2024

I assume this should be oriented towards the development/v3.0 branch.

I think that this is also a fix for 2.3, but would need new release to be affective

@goneall
Copy link
Member

goneall commented Feb 5, 2024

I added PR #877 which adds this file to the development/v3.0 branch.

Once that is merged, let's create a PR to add this change into v3.0 and leave this PR open and/or merge it in case we do a dot release for v2.3 (note: no additional releases are currently planned).

Copy link
Contributor

@kestewart kestewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pulling this together. Changes look good to me.

@kestewart kestewart added this to the 2.3 milestone Feb 5, 2024
@pombredanne
Copy link
Member

SPDX license ids are unique ignoring case in practice and the case of operators does not matter and does not change the meaning of anything, the case should not matter.

Therefore, my 2 cents would be to:

  1. Not care about the case at all at parsing time, be case insensitive.
  2. Have a canonical representation (say all uppercase, OR all lowercase, OR mixed uppercase for operators and lowercase for ids, or anything that is simple and foolproof).

If you want to make things more practical for adopters, you should consider these changes in 3.0. The proposed change here for 2.3 is neither bad, nor great and it creates unnecessary churn for all implementers: it would be best to do better and settle the case issue once for all to avoid further churn.

Being strict on which character case to use for SPDX operators and SPDX license ids has always been something that I found to be problematic with no actual value except making the experience of adopters and implementers more complex and error prone, and is something that I would qualify as a self-inflicted wound.

@pombredanne
Copy link
Member

Just to add another squeak to the wheel... this will likely make many tools stop working when they validate such expression, so this is likely an API-breaking change.

@maxhbr
Copy link
Member

maxhbr commented Feb 5, 2024

Just to add another squeak to the wheel... this will likely make many tools stop working when they validate such expression, so this is likely an API-breaking change.

Agreed, this is a breaking change for consumers.

@goneall
Copy link
Member

goneall commented Feb 6, 2024

Thanks @pombredanne and @maxhbr for pointing out the potential for breaking changes.

The parsing algorithms I build are case insensitive, so I kind of missed this in my reviews.

Two points:

  • Since it is breaking, we should not merge this into a dot release - we should probably close this PR
  • We should target this change to 3.0 (after a possibly conflicting PR Add back SPDX license expressions to version 3 branch #877 is reviewed and merged) where a breaking change is possible. I personally support the change, but I can understand where others may object due to the need to update tooling.

@xsuchy
Copy link
Author

xsuchy commented Feb 6, 2024

I agree, closing here. I will open new PR against 3.0 when #877 will be merged.

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 this pull request may close these issues.

6 participants