clean up enum and comma in annotation #159
Conversation
|
|
There was a problem hiding this comment.
@valulucchesi Thanks a lot for your contribution. Please find my comments on the pull request.
| String replacementString = state.getSourceForNode(tree); | ||
| if (index == resolvedTestAnnotation.getFlags().size() - 1) { | ||
| replacementString = | ||
| replacementString.replace(", " + state.getSourceForNode(expressionTree), ""); |
There was a problem hiding this comment.
While I believe this will work in most cases, it still ties us down to a formatting style where arguments are separated by , . I believe we should make it more general, (e.g. arguments could also be separated by a new line character or arguments could be separated by just a ,).
One options would be to do it the way error-prone folks delete type arguments in their UnnecessaryTypeArguments bug check.
| " @interface ToggleTesting {", | ||
| " TestExperimentName[] treated();", | ||
| " }", | ||
| " @ToggleTesting(treated = {TestExperimentName.STALE_FLAG, TestExperimentName.OTHER_FLAG})", |
There was a problem hiding this comment.
Another test scenario could be added @ToggleTesting(treated = {TestExperimentName.STALE_FLAG,TestExperimentName.OTHER_FLAG}) and
@ToggleTesting(treated = {TestExperimentName.STALE_FLAG,
TestExperimentName.OTHER_FLAG})
…eanup-enum-in-annotation-with-comma
| } | ||
| } | ||
| return fixBuilder; | ||
| } | ||
|
|
||
| private int getLower(CharSequence source, int lower, int index, int flagSize) { | ||
| while (lower >= 0 | ||
| && source.charAt(lower) != '{' |
There was a problem hiding this comment.
In what scenarios would we encounter a { or a = in an Enum List? Can you please add a test scenario for this?
There was a problem hiding this comment.
for = i was thinking in @ToggleTesting(treated = TestExperimentName.STALE_FLAG) but you are right that it is not needed at this stage so i'll remove it
for { the scenario is tested in testRemoveFirstEnumFromAnnotationRemovingCommaAndKeepingOtherEnum where the enum is at the beginning of the list: @ToggleTesting(treated = {TestExperimentName.STALE_FLAG, TestExperimentName.OTHER_FLAG})
|
Thanks a lot for your contribution and the comprehensive tests:) |
|
@valulucchesi I was wondering if this new implementation could also handle something like this ? |
when there is more than one enum in the annotation and it's being separated by comma piranha is keeping an extra comma after the cleanup.
code to do the cleanup by STALE_FLAG as treated:
@ToggleTesting(treated = {TestExperimentName.STALE_FLAG, TestExperimentName.OTHER_FLAG})current output:
@ToggleTesting(treated = {, TestExperimentName.OTHER_FLAG})expected output
@ToggleTesting(treated = {TestExperimentName.OTHER_FLAG})The enum to be removed could be at the beginning, in the middle or at the end. For the first two cases we should remove the comma that is present after the enum and for the third case we should remove the comma that is present before the enum.
For the scenario when there is only one enum we shouldn't do anything else that what the code is currently doing.