feat: initial implementation #1
Conversation
javier-godoy
force-pushed
the
initial-implementation
branch
2 times, most recently
from
e7d16cb to
a24eef1
Compare
javier-godoy
force-pushed
the
initial-implementation
branch
2 times, most recently
from
58ae78d to
82d2ba6
Compare
javier-godoy
changed the title
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionField.java
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionTestField.java
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldView.java
Outdated
Show resolved
Hide resolved
javier-godoy
force-pushed
the
initial-implementation
branch
from
82d2ba6 to
982600d
Compare
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java
Outdated
Show resolved
Hide resolved
javier-godoy
force-pushed
the
initial-implementation
branch
from
982600d to
dc2063f
Compare
There was a problem hiding this comment.
Please check punctuation in block tags like param and return as they are not supposed to end with period. See FlowingCode/DevelopmentConventions#49 (comment) Done.
github-project-automation
bot
moved this from To Do
to In Progress
in Flowing Code Addons
javier-godoy
force-pushed
the
initial-implementation
branch
from
dc2063f to
ce9c7ed
Compare
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionField.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionField.java
Outdated
Show resolved
Hide resolved
javier-godoy
force-pushed
the
initial-implementation
branch
from
ce9c7ed to
4768591
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis update introduces a new Vaadin add-on, "Regular Expression Field Add-on," replacing the previous "Template Add-on." It adds new Java classes for regular expression construction, validation, and testing UI components, updates documentation and configuration files to reflect the new add-on identity, and removes obsolete template-related files and tests. The Maven configuration is revised, and test/demo code is adapted for the new functionality. Changes
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/test/java/com/flowingcode/vaadin/addons/regex/test/SerializationTest.java (1)
45-51: Enhance serialization test with explicit assertions. Currently the test only fails on exceptions. To strengthen it, capture the result ofobj.getClass().cast(...)and add anAssert.assertNotNull(...)(or equivalent) to verify the object was deserialized correctly.README.md (1)
75-75: Inconsistent capitalization in author line. Change "Regular Expression Field Add-On" to "Regular Expression Field Add-on" to match branding elsewhere.src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldDemo.java (1)
41-51: Consider extracting the conditional debug code.The conditional compilation directive (#if vaadin eq 0) contains UI logic that might be better separated into a private method for clarity. This would improve code organization, especially if more debugging features are added in the future.
add(field); // #if vaadin eq 0 -Div div1 = new Div(); -Div div2 = new Div(); -add(div1, div2); -field.addValueChangeListener( - ev -> { - div1.setText(Optional.ofNullable(ev.getValue()).map(Object::toString).orElse("")); - div2.setText(Optional.ofNullable(ev.getValue()).map(RegularExpression::getPattern) - .map(Pattern::pattern).orElse("")); - }); +addDebugComponents(field); // #endif + +// #if vaadin eq 0 +private void addDebugComponents(RegularExpressionField field) { + Div div1 = new Div(); + Div div2 = new Div(); + add(div1, div2); + field.addValueChangeListener( + ev -> { + div1.setText(Optional.ofNullable(ev.getValue()).map(Object::toString).orElse("")); + div2.setText(Optional.ofNullable(ev.getValue()).map(RegularExpression::getPattern) + .map(Pattern::pattern).orElse("")); + }); +} +// #endifsrc/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionTestField.java (2)
53-101: Consider extracting complex grid setup logic to separate methods.The grid setup and configuration logic is quite extensive. Consider refactoring this by extracting different aspects (editor setup, button creation, event handling) into separate private methods to improve readability and maintainability.
For example, you could extract event handlers to separate methods:
- grid.addItemClickListener(ev -> grid.getEditor().editItem(ev.getItem())); - editField.addBlurListener(ev -> { - grid.getEditor().closeEditor(); - }); - grid.getEditor().addCloseListener(ev -> { - if (ev.getItem()[0].isEmpty()) { - grid.getListDataView().removeItem(ev.getItem()); - } - }); - grid.getEditor().addOpenListener(ev -> { - editField.focus(); - }); + setupGridListeners(grid, editField);And add the extracted method:
private void setupGridListeners(Grid<String[]> grid, TextField editField) { grid.addItemClickListener(ev -> grid.getEditor().editItem(ev.getItem())); editField.addBlurListener(ev -> grid.getEditor().closeEditor()); grid.getEditor().addCloseListener(ev -> { if (ev.getItem()[0].isEmpty()) { grid.getListDataView().removeItem(ev.getItem()); } }); grid.getEditor().addOpenListener(ev -> editField.focus()); }
110-129: Consider adding null checks for pattern parameter.The setPattern method doesn't explicitly check if the pattern is null before using it. Although the current implementation handles null through Optional in the RegularExpression method, it's good practice to include explicit validation.
public void setPattern(Pattern pattern) { + // null pattern will clear the current pattern this.pattern = pattern; grid.getDataProvider().refreshAll(); }src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionField.java (2)
57-68: Initialize testFieldEnabled in constructor.The visibility of the testField depends on testFieldEnabled, but testFieldEnabled is not explicitly initialized in the constructor. While it defaults to false as a class field, consider initializing it explicitly in the constructor for clarity.
public RegularExpressionField() { operatorField = new Select<>(); operatorField.setItems(RegularExpressionOperator.values()); inputField = new TextField(); testField = new RegularExpressionTestField(); testField.setVisible(false); + testFieldEnabled = false; add(new HorizontalLayout(operatorField, inputField), testField); operatorField.addValueChangeListener(ev -> setTestFieldEnabled(testFieldEnabled)); addValueChangeListener(ev -> testField.setPattern(getValue())); }
70-88: Simplify generateModelValue by handling empty fields first.The method can be simplified by restructuring to handle empty fields case first, which would reduce nesting and improve readability.
@Override protected RegularExpression generateModelValue() { if (hasPatternSyntaxError) { onPatternSyntaxException(null, 0); hasPatternSyntaxError = false; } - if (!inputField.isEmpty() && !operatorField.isEmpty()) { - try { - var r = new RegularExpression(operatorField.getValue(), inputField.getValue()); - return r; - } catch (PatternSyntaxException e) { - onPatternSyntaxException(e.getDescription(), e.getIndex()); - hasPatternSyntaxError = true; - return null; - } - } else { + if (inputField.isEmpty() || operatorField.isEmpty()) { return null; + } + + try { + var r = new RegularExpression(operatorField.getValue(), inputField.getValue()); + return r; + } catch (PatternSyntaxException e) { + onPatternSyntaxException(e.getDescription(), e.getIndex()); + hasPatternSyntaxError = true; + return null; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
README.md(3 hunks)pom.xml(6 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionField.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionOperator.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionTestField.java(1 hunks)src/main/resources/META-INF/VAADIN/package.properties(0 hunks)src/main/resources/META-INF/frontend/styles/fc-regex-test-strings.css(1 hunks)src/main/resources/META-INF/frontend/styles/static_addon_styles(0 hunks)src/main/resources/META-INF/resources/static_addon_resources(0 hunks)src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/DemoView.java(3 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldDemo.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldView.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/test/RegularExpressionTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/test/SerializationTest.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/template/TemplateDemo.java(0 hunks)src/test/java/com/flowingcode/vaadin/addons/template/it/AbstractViewTest.java(0 hunks)src/test/java/com/flowingcode/vaadin/addons/template/it/ViewIT.java(0 hunks)src/test/resources/META-INF/frontend/styles/shared-styles.css(0 hunks)src/test/resources/META-INF/resources/static_addon_test_resources(0 hunks)
💤 Files with no reviewable changes (8)
- src/test/resources/META-INF/resources/static_addon_test_resources
- src/main/resources/META-INF/frontend/styles/static_addon_styles
- src/main/resources/META-INF/resources/static_addon_resources
- src/main/resources/META-INF/VAADIN/package.properties
- src/test/resources/META-INF/frontend/styles/shared-styles.css
- src/test/java/com/flowingcode/vaadin/addons/template/TemplateDemo.java
- src/test/java/com/flowingcode/vaadin/addons/template/it/ViewIT.java
- src/test/java/com/flowingcode/vaadin/addons/template/it/AbstractViewTest.java
🔇 Additional comments (30)
src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java (1)
3-6: License header updated correctly. The add-on name and copyright year have been updated consistently with the new branding.src/test/java/com/flowingcode/vaadin/addons/regex/test/SerializationTest.java (2)
3-6: License header updated correctly. The add-on name and copyright year have been bumped to 2025.
20-23: Package and import adjustments are accurate. The test now references the newcom.flowingcode.vaadin.addons.regexpackage and importsRegularExpressionField.src/test/java/com/flowingcode/vaadin/addons/regex/DemoView.java (3)
3-6: License header updated to new add-on. The header reflects the renamed add-on and updated year.
21-21: Package declaration updated. Moved to theregexnamespace to match the component’s package.
34-34: Navigation target updated. Forwarding toRegularExpressionFieldViewaligns the demo with the new view class.src/main/resources/META-INF/frontend/styles/fc-regex-test-strings.css (2)
3-6: License header updated. The file header now correctly names the Regular Expression Field Add-on and year.
20-40: New CSS rules look correct. Styles for grid cells, buttons, and feedback parts are defined properly and leverage Lumo tokens appropriately.README.md (5)
1-5: Badges updated successfully. All directory, build, and Maven Central badges point to the new regular-expression-field-add-on artifact.
7-7: Project title updated. The heading correctly identifies the add-on as "Regular Expression Field Add-on."
13-15: Features section revised. The new bullet points accurately describe the simple and advanced regex capabilities.
20-20: Demo link updated. The online demo URL now reflects the regular-expression-field path.
33-33: Dependency artifactId updated. The Maven coordinates now useregular-expression-field-addon.src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionOperator.java (1)
20-42: Well-structured enum definition with clear documentation.The enum definition is clean, concise, and follows Java best practices. Each operator's purpose is clearly documented, making the API easy to understand for developers.
src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldView.java (2)
31-32: GitHub link has been properly updated.The GitHub link has been updated to reflect the new repository location, addressing the previous review comment.
34-37: Class constructor properly initializes the demo.The constructor properly initializes the TabbedDemo with the appropriate demo class for the regular expression field.
src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldDemo.java (1)
35-40: Well-structured demo initialization with test data.The demo initializes the RegularExpressionField with an appropriate test pattern and sample strings, providing a good demonstration of the component's functionality.
pom.xml (4)
15-15: Vaadin version updated appropriately.Upgrading from 24.4.6 to 24.7.0 ensures the add-on uses a more recent version of the framework.
124-129: Lombok dependency added with appropriate scope.The addition of Lombok with 'provided' scope is appropriate, ensuring it's available during compile time but not included in the final artifact.
275-277: Frontend hotdeploy property correctly moved to system properties.Moving the frontend.hotdeploy property to the Jetty plugin's systemProperties section ensures it's properly set when Jetty runs.
548-562: Antrun tasks updated with proper frontend resource handling.The Maven Antrun plugin configuration has been updated to properly handle frontend resources, ensuring they're correctly placed for the demo-war build profile.
src/test/java/com/flowingcode/vaadin/addons/regex/test/RegularExpressionTest.java (4)
28-39: Well-structured test cases with comprehensive assertions.The test structure is clear and effective, with proper assertions using Hamcrest matchers to validate both the pattern generation and the input preservation. The test also verifies the reconstruction from a Pattern back to a RegularExpression instance.
41-50: Good test for special character escaping in STARTS_WITH mode.This test properly verifies that the square bracket character is correctly escaped when used with the STARTS_WITH operator, and that the original input is preserved. The pattern generation and round-trip conversion are both validated.
85-94: Thorough handling of edge case with escape sequences.The test effectively validates the correct handling of a complex case involving the
\Eescape sequence, which is a potential edge case in regular expression quoting.
118-129: Excellent coverage of regular expression metacharacters.The test iterates through all common regex metacharacters to ensure they're properly escaped in CONTAINS mode patterns. This is a comprehensive approach to testing character escaping.
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionTestField.java (2)
38-46: Well-documented class purpose with clear Javadoc.The class documentation clearly describes its purpose and functionality, addressing the previous review comment.
93-99: Good use of part name generator for visual feedback.The implementation using setPartNameGenerator to provide visual feedback on regex matching is clean and effective. This allows CSS styling to be applied based on match results.
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java (2)
35-47: Well-documented class with clear Javadoc.The class documentation clearly describes its purpose and functionality, addressing the previous review comment.
93-108: Clever optimization for regex quoting strategies.The quote method intelligently selects between two different quoting strategies based on which produces the shorter result. This optimization helps keep regular expressions as concise as possible.
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionField.java (1)
29-42: Well-documented class with comprehensive Javadoc.The class documentation clearly describes its purpose and features, addressing the previous review comment.
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java
Show resolved
Hide resolved
javier-godoy
force-pushed
the
initial-implementation
branch
from
4768591 to
cfe0278
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java (1)
126-128: Null check implemented correctly.The null check for the pattern parameter has been properly implemented as suggested in a previous review.
🧹 Nitpick comments (3)
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java (3)
76-89: Consider validating empty input strings.The constructor accepts empty input strings, which might produce unexpected regex patterns for operators like CONTAINS, STARTS_WITH, and ENDS_WITH. Consider adding validation to ensure input is not empty for these operators.
public RegularExpression(@NonNull RegularExpressionOperator operator, @NonNull String input) throws PatternSyntaxException { this.operator = operator; this.input = input; + // Validate input for non-ADVANCED operators + if (operator != ADVANCED && input.isEmpty()) { + throw new IllegalArgumentException("Input string cannot be empty for " + operator + " operator"); + } String regex = switch (operator) { case ADVANCED -> input; case CONTAINS -> ANY + quote(input) + ANY; case ENDS_WITH -> ANY + quote(input); case STARTS_WITH -> quote(input) + ANY; }; pattern = Pattern.compile(regex); }
91-106: Improve robustness of pattern creation.The current approach of using string replacement to incorporate the CHARS constant into pattern definitions could be error-prone if CHARS changes. Consider using Pattern.quote() for simpler cases or creating a more robust pattern builder.
private final static String CHARS = ".?+*\\[({$^|\\\\"; -private final static Pattern SIMPLE_PATTERN = - Pattern.compile("(?:[^CHARS]|\\\\[CHARS])+".replace("CHARS", CHARS)); - -private final static Pattern ESCAPE_PATTERN = - Pattern.compile("[CHARS]".replace("CHARS", CHARS)); - -private final static Pattern UNQUOTE_PATTERN = - Pattern.compile("\\\\([CHARS])".replace("CHARS", CHARS)); +// Improved pattern definitions for better readability and maintainability +private final static Pattern SIMPLE_PATTERN = Pattern.compile( + "(?:[^" + CHARS + "]|\\\\" + "[" + CHARS + "])+"); + +private final static Pattern ESCAPE_PATTERN = Pattern.compile( + "[" + CHARS + "]"); + +private final static Pattern UNQUOTE_PATTERN = Pattern.compile( + "\\\\([" + CHARS + "])");
124-163: Improve readability of the factory method.The factory method contains complex logic that could benefit from more descriptive variable names and possibly breaking down into smaller methods.
public static RegularExpression of(Pattern pattern) { if (pattern == null) { throw new NullPointerException("Pattern cannot be null"); } String regex = pattern.pattern(); - boolean notStartsWith = false; - boolean notEndsWith = false; + boolean hasLeadingWildcard = false; + boolean hasTrailingWildcard = false; if (regex.startsWith(ANY)) { - notStartsWith = true; + hasLeadingWildcard = true; regex = regex.substring(2); } if (regex.endsWith(ANY)) { - notEndsWith = true; + hasTrailingWildcard = true; regex = regex.substring(0, regex.length() - 2); } String input = null; - if (notStartsWith || notEndsWith) { + if (hasLeadingWildcard || hasTrailingWildcard) { if (SIMPLE_PATTERN.matcher(regex).matches()) { input = UNQUOTE_PATTERN.matcher(regex).replaceAll("$1"); } else if (regex.startsWith("\\Q") && regex.endsWith("\\E")) { input = regex.substring(2, regex.length() - 2).replace("\\E\\\\E\\Q", "\\E"); } } if (input != null) { - if (notStartsWith && notEndsWith) { + if (hasLeadingWildcard && hasTrailingWildcard) { return new RegularExpression(CONTAINS, input); - } else if (notStartsWith) { + } else if (hasLeadingWildcard) { return new RegularExpression(ENDS_WITH, input); - } else if (notEndsWith) { + } else if (hasTrailingWildcard) { return new RegularExpression(STARTS_WITH, input); } } return new RegularExpression(ADVANCED, pattern.pattern(), pattern); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
README.md(3 hunks)pom.xml(6 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionField.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionOperator.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionTestField.java(1 hunks)src/main/resources/META-INF/VAADIN/package.properties(0 hunks)src/main/resources/META-INF/frontend/styles/fc-regex-test-strings.css(1 hunks)src/main/resources/META-INF/frontend/styles/static_addon_styles(0 hunks)src/main/resources/META-INF/resources/static_addon_resources(0 hunks)src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/DemoView.java(3 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldDemo.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldView.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/test/RegularExpressionTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/test/SerializationTest.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/template/TemplateDemo.java(0 hunks)src/test/java/com/flowingcode/vaadin/addons/template/it/AbstractViewTest.java(0 hunks)src/test/java/com/flowingcode/vaadin/addons/template/it/ViewIT.java(0 hunks)src/test/resources/META-INF/frontend/styles/shared-styles.css(0 hunks)src/test/resources/META-INF/resources/static_addon_test_resources(0 hunks)
💤 Files with no reviewable changes (8)
- src/test/resources/META-INF/frontend/styles/shared-styles.css
- src/main/resources/META-INF/resources/static_addon_resources
- src/test/resources/META-INF/resources/static_addon_test_resources
- src/main/resources/META-INF/frontend/styles/static_addon_styles
- src/main/resources/META-INF/VAADIN/package.properties
- src/test/java/com/flowingcode/vaadin/addons/template/TemplateDemo.java
- src/test/java/com/flowingcode/vaadin/addons/template/it/ViewIT.java
- src/test/java/com/flowingcode/vaadin/addons/template/it/AbstractViewTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionTestField.java
🚧 Files skipped from review as they are similar to previous changes (11)
- src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java
- src/test/java/com/flowingcode/vaadin/addons/regex/test/SerializationTest.java
- src/test/java/com/flowingcode/vaadin/addons/regex/DemoView.java
- src/main/resources/META-INF/frontend/styles/fc-regex-test-strings.css
- src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionOperator.java
- src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldDemo.java
- src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldView.java
- README.md
- pom.xml
- src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionField.java
- src/test/java/com/flowingcode/vaadin/addons/regex/test/RegularExpressionTest.java
🔇 Additional comments (2)
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java (2)
49-65: LGTM - Good immutable class design!The class is properly designed as immutable with final fields and no setters. The use of Lombok annotations reduces boilerplate while maintaining clean semantics.
34-48: Well-documented class with clear purpose.The class documentation clearly explains the purpose and behavior of the RegularExpression class, addressing the previous review comment.
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java
Show resolved
Hide resolved
javier-godoy
force-pushed
the
initial-implementation
branch
from
cfe0278 to
cc846e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java (1)
1-170: Consider adding validation tests for edge casesAs mentioned in a previous review comment, this class would benefit from comprehensive unit tests for edge cases.
Ensure that you've implemented unit tests for:
- Empty input strings with different operators
- Input containing special regex characters
- Long patterns
- Complex escaping sequences
- Round-trip conversion tests
These tests are crucial to verify the robustness of this implementation.
🧹 Nitpick comments (3)
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java (3)
102-106: Add more detailed documentation for the quote methodThe
quotemethod intelligently chooses between two quoting strategies to optimize the pattern's readability and length, but this logic isn't documented./** + * Quotes a string for use in a regular expression. + * + * <p>This method selects the more efficient quoting strategy between: + * <ol> + * <li>Escaping individual special characters</li> + * <li>Using \Q...\E literal quotation with special handling for \E sequences</li> + * </ol> + * The method chooses the strategy that results in the shorter output string. + * + * @param input the string to be quoted + * @return the quoted string ready for use in a regex pattern + */ private static String quote(String input) { String s1 = ESCAPE_PATTERN.matcher(input).replaceAll("\\\\$0"); String s2 = "\\Q" + input.replace("\\E", "\\E\\\\E\\Q") + "\\E"; return (s1.length() < s2.length()) ? s1 : s2; }
124-163: Enhance documentation for pattern analysis in the of() methodThe pattern reverse-engineering logic is quite complex but lacks detailed documentation explaining the steps involved in determining the operator type.
The method already has good high-level documentation, but adding comments within the method would make the implementation more maintainable:
public static RegularExpression of(Pattern pattern) { if (pattern == null) { throw new NullPointerException("Pattern cannot be null"); } String regex = pattern.pattern(); boolean hasLeadingWildcard = false; boolean hasTrailingWildcard = false; + // Check if the pattern starts with a wildcard (.*) and remove it if present if (regex.startsWith(ANY)) { hasLeadingWildcard = true; regex = regex.substring(2); } + // Check if the pattern ends with a wildcard (.*) and remove it if present if (regex.endsWith(ANY)) { hasTrailingWildcard = true; regex = regex.substring(0, regex.length() - 2); } String input = null; if (hasLeadingWildcard || hasTrailingWildcard) { + // Try to extract the input string by checking if it's a simple pattern + // or a pattern enclosed in \Q...\E quotation markers if (SIMPLE_PATTERN.matcher(regex).matches()) { input = UNQUOTE_PATTERN.matcher(regex).replaceAll("$1"); } else if (regex.startsWith("\\Q") && regex.endsWith("\\E")) { input = regex.substring(2, regex.length() - 2).replace("\\E\\\\E\\Q", "\\E"); } } + // Determine which operator to use based on the wildcard positions if (input != null) { if (hasLeadingWildcard && hasTrailingWildcard) { return new RegularExpression(CONTAINS, input); } else if (hasLeadingWildcard) { return new RegularExpression(ENDS_WITH, input); } else if (hasTrailingWildcard) { return new RegularExpression(STARTS_WITH, input); } } + // Default to advanced mode if we couldn't determine a simple operator return new RegularExpression(ADVANCED, pattern.pattern(), pattern); }
91-101: Document the regex pattern constantsThe constants for regex patterns lack documentation explaining their purpose and usage.
+ /** Characters that need special handling in regex patterns */ private final static String CHARS = ".?+*\\[({$^|\\\\"; + /** Pattern for identifying a simple regex containing only literal characters or escaped special characters */ private final static Pattern SIMPLE_PATTERN = Pattern.compile("(?:[^CHARS]|\\\\[CHARS])+".replace("CHARS", CHARS)); + /** Pattern for identifying special regex characters that need escaping */ private final static Pattern ESCAPE_PATTERN = Pattern.compile("[CHARS]".replace("CHARS", CHARS)); + /** Pattern for identifying already escaped special characters to be unescaped */ private final static Pattern UNQUOTE_PATTERN = Pattern.compile("\\\\([CHARS])".replace("CHARS", CHARS));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
README.md(3 hunks)pom.xml(6 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionField.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionOperator.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionTestField.java(1 hunks)src/main/resources/META-INF/VAADIN/package.properties(0 hunks)src/main/resources/META-INF/frontend/styles/fc-regex-test-strings.css(1 hunks)src/main/resources/META-INF/frontend/styles/static_addon_styles(0 hunks)src/main/resources/META-INF/resources/static_addon_resources(0 hunks)src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/DemoView.java(3 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldDemo.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldView.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/test/RegularExpressionTest.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/regex/test/SerializationTest.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/template/TemplateDemo.java(0 hunks)src/test/java/com/flowingcode/vaadin/addons/template/it/AbstractViewTest.java(0 hunks)src/test/java/com/flowingcode/vaadin/addons/template/it/ViewIT.java(0 hunks)src/test/resources/META-INF/frontend/styles/shared-styles.css(0 hunks)src/test/resources/META-INF/resources/static_addon_test_resources(0 hunks)
💤 Files with no reviewable changes (8)
- src/test/resources/META-INF/resources/static_addon_test_resources
- src/main/resources/META-INF/VAADIN/package.properties
- src/main/resources/META-INF/frontend/styles/static_addon_styles
- src/test/resources/META-INF/frontend/styles/shared-styles.css
- src/main/resources/META-INF/resources/static_addon_resources
- src/test/java/com/flowingcode/vaadin/addons/template/TemplateDemo.java
- src/test/java/com/flowingcode/vaadin/addons/template/it/ViewIT.java
- src/test/java/com/flowingcode/vaadin/addons/template/it/AbstractViewTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionTestField.java
🚧 Files skipped from review as they are similar to previous changes (11)
- src/main/resources/META-INF/frontend/styles/fc-regex-test-strings.css
- src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java
- src/test/java/com/flowingcode/vaadin/addons/regex/test/SerializationTest.java
- README.md
- src/test/java/com/flowingcode/vaadin/addons/regex/DemoView.java
- src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionOperator.java
- src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpressionField.java
- src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldView.java
- src/test/java/com/flowingcode/vaadin/addons/regex/RegularExpressionFieldDemo.java
- src/test/java/com/flowingcode/vaadin/addons/regex/test/RegularExpressionTest.java
- pom.xml
🔇 Additional comments (4)
src/main/java/com/flowingcode/vaadin/addons/regex/RegularExpression.java (4)
34-46: Javadoc well-documented addressing previous feedbackThe class Javadoc clearly explains the purpose and behavior of the RegularExpression class, addressing the previous review comment about including a description of the class purpose.
126-128: Good null check implementationThe null check for the pattern parameter properly addresses the previous code review comment.
49-51: Appropriate Lombok annotations for immutabilityThe class correctly uses Lombok annotations to implement an immutable class pattern. The
@Setterannotation has been removed as suggested in a previous review.
81-89: Well-implemented switch expression with proper pattern handlingThe switch expression nicely handles the different operators and generates the appropriate regex pattern for each case. The code leverages modern Java language features effectively.
github-project-automation
bot
moved this from In Progress
to Pending release
in Flowing Code Addons
https://github.com/FlowingCode/AddonsInternal/issues/19
Summary by CodeRabbit
New Features
Documentation
Style
Tests
Chores