Remove regex pattern from ParsingUtils #366
Conversation
Signed-off-by: Fynn Juranek <[email protected]>
Signed-off-by: Fynn Juranek <[email protected]>
|
|
||
| public class ParsingUtils { | ||
|
|
||
| private ParsingUtils() { |
There was a problem hiding this comment.
I don't think anyone should have created an instance for this but strictly, this is a breaking change. I think we should fix this but maybe in another PR so we can call it out in the release notes?
There was a problem hiding this comment.
I mean who would ever want to instantiate this. I think that most IDEs suggest doing the private constructor. We can of course ignore this for the next minor and just not add this constructor.
There was a problem hiding this comment.
I think it is probably ok to break this in a minor version. I would just do it in a separate PR so it is easier to call out in the release notes and separate it from an enhancement PR.
There was a problem hiding this comment.
Agree. This is resolved with commit: d879fba7.
Do you want me to open a PR for the private Constructor?
| JavaClassSource javaSource = RoasterTestUtils.readJavaClass(ParsingUtilsReadEnumClassNamesTests.class); | ||
| MethodSource<?> methodSource = ((JavaClassSource) javaSource.getNestedType("MyClass")).getMethod("mergeNested"); | ||
| Set<String> result = ParsingUtils.readEnumClassNames(methodSource); | ||
| assertThat(result).containsExactlyInAnyOrder("NestedFooKeyName", "NestedBarKeyName"); |
There was a problem hiding this comment.
Is this correct?
I'm questioning every nested class in this test.
Since the class is nested, it's name is not NestedFooKeyName but ParsingUtilsReadEnumClassNamesTests$FooMeterDocumentation$NestedFooKeyName or ParsingUtilsReadEnumClassNamesTests.FooMeterDocumentation.NestedFooKeyName depending on the context.
There was a problem hiding this comment.
This is currently correct, as I add only the last part of the nested KeyNames, which happens here: ParsingUtils.java#L119. But I could just get rid of the substringing and add the whole class name, which would probably lead to a clearer description of where the nested KeyName is coming from.
There was a problem hiding this comment.
Changed this now with commit: 213d0386. But this still does not take ParsingUtilsReadEnumClassNamesTests into place as we provide only the the nested type "MyClass" as the methodsource which does not contain ParsingUtilsReadEnumClassNamesTests. Because of these two lines: ParsingUtilsReadEnumClassNamesTests.java#L57-L58, I could change this to include the "root" source which would probably need a different jboss method.
Do you want me to change that?
- Remove as this would silently introduce a breaking change Signed-off-by: Fynn Juranek <[email protected]>
Signed-off-by: Fynn Juranek <[email protected]>
3 participants
The solution involves replacing the regex pattern usage in ParsingUtils with AST parsing, similar to how we retrieve expressions.
The existing functionality remains unchanged and additionally, there is a new test case added to ensure everything works with nested KeyNames.
closes gh-365