finish ToDo in localizationParameterMustIncludeAString #11793
Conversation
1. add StringUtils.countMatches(e.getKey(), "\"") == 2 to cover Localization.lang(var1 + "test" + var2) case 2. add link and some information to comment
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| keys = LocalizationParser.findLocalizationParametersStringsInJavaFiles(LocalizationBundleForTest.MENU); | ||
| for (LocalizationEntry e : keys) { | ||
| assertTrue(e.getKey().startsWith("\"") || e.getKey().endsWith("\""), "Illegal localization parameter found. Must include a String with potential concatenation or replacement parameters. Illegal parameter: Localization.lang(" + e.getKey()); | ||
| assertTrue(e.getKey().startsWith("\"") || e.getKey().endsWith("\"") || StringUtils.countMatches(e.getKey(), "\"") == 2, "Illegal localization parameter found. Must include a String with potential concatenation or replacement parameters. Illegal parameter: Localization.lang(" + e.getKey()); |
There was a problem hiding this comment.
One day later, I have a more clear mind.
We have
Localization.lang("Added group \"%0\".", group.getName())
Localiatzion:
Added group "%0".
It has two ". This is OK.
The updated code of you checks for two of them. It should match. It does not.
I think, more debugging and thinking is required here.
Maybe, a test for the test needs to be developed?
It is OK for a coding excercise, but not really required for JabRef. We have many other user-facing issues. We should work on these.
There was a problem hiding this comment.
One day later, I have a more clear mind.
We have
Localization.lang("Added group \"%0\".", group.getName())Localiatzion:
Added group "%0".It has two
". This is OK.The updated code of you checks for two of them. It should match. It does not.
I think, more debugging and thinking is required here.
Maybe, a test for the test needs to be developed?
It is OK for a coding excercise, but not really required for JabRef. We have many other user-facing issues. We should work on these.
OK, I'll try to find a proper user-facing issue to solve from tomorrow
There was a problem hiding this comment.
Localization.lang("Added group "%0".", group.getName())
this case can be covered by e.getKey().startsWith("\""), so that is why my code can pass the unit test phase in CI/CD workflow or local test run.
There was a problem hiding this comment.
The || in the condition feels wrong.
Localization.lang(var1 + "test" + var2)
Matches this condition, but is forbidden.
There was a problem hiding this comment.
The
||in the condition feels wrong.Localization.lang(var1 + "test" + var2)Matches this condition, but is forbidden.
the original ToDo is TODO: Localization.lang(var1 + "test" + var2) not covered, I thought it means Localization.lang(var1 + "test" + var2) is not forbidden.
Then just delete the ToDo in comment like option 2 in #11784 or I just close this PR?
There was a problem hiding this comment.
Hey, sorry to have kept you waiting. We put through a refinement at #11820 and thus decided on closing this one.
Thanks for taking this up!
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)