-
Notifications
You must be signed in to change notification settings - Fork 26
Change to JSON-formatted expected warnings #216
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
Conversation
Signed-off-by: Steve Winslow <steve@swinslow.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original functionality was to ignore any warning strings - not just the duplicate IDs.
It's been some time since I've looked at the code - it's quite possible that the only warnings are duplicate ID's but I would need to check.
Perhaps we can accomodate other warnings by making the JSON file a bit more descriptive by having a property "duplicateIds" with the array and then having another property "additionalWarnings" which would have additional strings.
We should also document the format in the usage field of the command line.
|
Thanks @goneall! I'll take a look at this -- apologies for the delayed response as I was away and traveling much of last week. Understood on the goal for adding extensibility here, and I'll take a look to revise it. |
Signed-off-by: Steve Winslow <steve@swinslow.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Thanks @swinslow
|
@swinslow - let me know once the license list XML repo changes are approved - after that, I'll go ahead and merge and spin another release. |
|
@swinslow - Has the license list XML website been updated? Thinking of spinning another release and would like to include this PR if it's OK to merge. |
|
+1, I'm good with merging this. Happy to get it in and test things out before we push the next release (sometime after end of October). Thank you @goneall! |
|
I forgot I need to update spdx/license-list-XML#2772 first to align with the changes in #216. Ugh, sorry. Will try to do so tomorrow on my way back from the conference... |
|
Attached is an updated JSON file that works with spdx/license-list-XML#2772 |
|
I tested this with the following JSON file: It all works good - I'll go ahead and merge. I can add the JSON file to the license-list-XML repo when I update the tool if @swinslow doesn't have time to update. |
This is a draft PR for changing to a JSON-formatted version of the
expected-warningsfile. It corresponds to the changes proposed in spdx/license-list-XML#2772.Please do not merge this until we've gotten the 3.27.0 license list release out the door, which should be within the next day or two.3.27.0 release is out!Please see the PR at spdx/license-list-XML#2772 for the proposed changes to the
expected-warningsfile. This PR for the publisher in turn swaps out the CSV reader for thegsonJSON reader as a new dependency.The code change in this PR then walks through the parsed arrays of related license IDs. For each pair in an array, it creates two new "Duplicates licenses:" entries, one for each ordering of the two IDs.
In this way, it should work as a drop-in replacement for the existing process for matching warnings. It just means that the "Duplicates licenses:" text lines are generated dynamically from the JSON at publisher run-time, rather than needing to maintain each of the strings in the XML repo's "source" files.
This is the first time in a long time that I've written Java code of any substance, so please take a close read of this and feel free to reject or replace it. :) Happy to discuss if you have any questions.
Signed-off-by: Steve Winslow steve@swinslow.net