Redesign the match within text #224
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
|
@pmonks - Feel free to take a look and let me know if you see any improvements. It removes a lot of the code and is much less complex in terms of code complexity metrics. |
pmonks
reviewed
Dec 15, 2023
There was a problem hiding this comment.
Looks good @goneall! I added some comments inline, but none of them are "showstoppers" - more like just observations of the code from a potential user's perspective (I was trying to put myself in the shoes of someone who might want to use this class without using LicenseCompareHelper).
[edit] oh and should there be a unit test class for this new class? One that would exercise the new class with specific fake "templates" that exercise different tricky parts of regex construction?
src/main/java/org/spdx/utility/compare/TemplateRegexGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/spdx/utility/compare/TemplateRegexGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/spdx/utility/compare/TemplateRegexGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/spdx/utility/compare/TemplateRegexGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/spdx/utility/compare/TemplateRegexGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/spdx/utility/compare/TemplateRegexGenerator.java
Outdated
Show resolved
Hide resolved
Also addresses review comments Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
|
@pmonks Request for an opinion on this PR. There are a few methods in the Do you think it is safe to delete? Here's the list of methods in
I've also done some further refactoring, so please take another look. The code should also be working, so if you could run a pass of your local tests (performance etc.) on this branch you may catch something I missed. |
|
@goneall rushing out the door, so can't do a deep dive, but perhaps we could just I should be able to run the full test suite tonight. |
|
@goneall I ran the existing unit tests with the "run slow tests" environment variable turned on, and got this promising result: So it's passing all of the existing tests, though I can't recall how well that exercises some of the bugs that we believe were fixed via the improved regexes in v1.1.9. I'll try to take a look at that next week, if you don't beat me to it (it seems like merging this change and releasing a v1.1.10 or whatever is kind of time sensitive, given the misbehaviour with the |
|
Oh and I also re-ran my little timing test (using the Results of that test: |
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
This was referenced Dec 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is currently a work in progress redesign of the match license / expression within text.
It creates a new class
TemplexRegexGeneratorwhich replaces much of the static methods and optional filtering class of the previous implementation.There is currently one unit test failing, so it is not quite ready to merge.