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

isStandardLicenseWithinText - match spaces after var rule #249

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

goneall
Copy link
Member

@goneall goneall commented Sep 7, 2024

Fixes #241
Fixes #234

Include code provided by @pmonk in #236 modified for version 2.0+

Thanks @pmonks and @sdheh for your patience and analysis / unit test help!

Fixes #241
Fixes #234

Include code provided by @pmonk in #236 modified for version 2.0+

Signed-off-by: Gary O'Neall <[email protected]>
@pmonks
Copy link
Collaborator

pmonks commented Sep 7, 2024

@goneall it appears that the new unit tests that leverage the CompareConsistencyHelper test class didn't make it over yet. Am I just jumping the gun, or was that an oversight?

FWIW here are the (minimal) unit tests in the original PR, though arguably more could/should be added (especially to cover the cases @sdheh identified in issue #245).

@goneall
Copy link
Member Author

goneall commented Sep 9, 2024

@goneall it appears that the new unit tests that leverage the CompareConsistencyHelper test class didn't make it over yet. Am I just jumping the gun, or was that an oversight?

FWIW here are the (minimal) unit tests in the original PR, though arguably more could/should be added (especially to cover the cases @sdheh identified in issue #245).

Thanks for the catch @pmonks - it must have gotten dropped when I rebased / merged it to the 2.0 latest.

I'll check to make sure the unit tests pass and add the tests back in in a subsequent commit.

Signed-off-by: Gary O'Neall <[email protected]>
@goneall
Copy link
Member Author

goneall commented Sep 9, 2024

@pmonks - the unit tests are now included - it passes with the SPDX_JAVA_LIB_RUN_SLOW_TESTS environment variable set to true.

@goneall
Copy link
Member Author

goneall commented Sep 18, 2024

@pmonks - Could you take another look and let me know if this is good to merge?

There's also a couple other PR's if you have time for a review.

@pmonks
Copy link
Collaborator

pmonks commented Sep 21, 2024

@goneall will do. Still just digging out after 3 weeks away.

@goneall
Copy link
Member Author

goneall commented Sep 21, 2024

@goneall will do. Still just digging out after 3 weeks away.

No worries - just wanted to keep it in your stack.

Welcome back.

Copy link
Collaborator

@pmonks pmonks left a comment

Choose a reason for hiding this comment

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

Looks good to me! I ran the unit tests and they're now passing (which they previously didn't). I also confirmed that the tests include the failure case identified in #245.

@pmonks
Copy link
Collaborator

pmonks commented Sep 23, 2024

@goneall one question though - I wasn't sure what the significance of org.spdx.utility.compare.LicenseCompareHelperTest vs org.spdx.utility.compare.LicenseCompareHelperTestV2 was. Is it just to split up what was becoming an unwieldy test case class, or something else? 🤔

@goneall
Copy link
Member Author

goneall commented Sep 23, 2024

@goneall one question though - I wasn't sure what the significance of org.spdx.utility.compare.LicenseCompareHelperTest vs org.spdx.utility.compare.LicenseCompareHelperTestV2 was. Is it just to split up what was becoming an unwieldy test case class, or something else? 🤔

Mostly to split up a large unwieldy test case class - the size of the class would essentially double when testing for both the compatible V2 license classes and the newer V3 classes. It also helps organize the imports (you'd need to fully qualify the class names due to similar class names in V2 and V3).

@goneall
Copy link
Member Author

goneall commented Sep 23, 2024

Looks good to me! I ran the unit tests and they're now passing (which they previously didn't). I also confirmed that the tests include the failure case identified in #245.

Thanks @pmonks

@goneall goneall merged commit f60be43 into master Sep 23, 2024
1 check passed
@goneall goneall deleted the spaceafterregex branch September 23, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants