Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish ToDo in localizationParameterMustIncludeAString #11784

Closed
leaf-soba opened this issue Sep 18, 2024 · 2 comments · Fixed by #11820
Closed

Finish ToDo in localizationParameterMustIncludeAString #11784

leaf-soba opened this issue Sep 18, 2024 · 2 comments · Fixed by #11820

Comments

@leaf-soba
Copy link
Contributor

leaf-soba commented Sep 18, 2024

Is your suggestion for improvement related to a problem? Please describe.

  1. I saw a ToDo in this unit test, said "TODO: Localization.lang(var1 + "test" + var2) not covered", and I want to finish it.
  2. I checked all strings in Localization.lang parameter in repo now, only got string begin with ".
  3. It means we don't have case Localization.lang(var + "test") or Localization.lang(var1 + "test" + var2) in repo, do I need to cover them when I finish the ToDo?
  4. I think only when developers need string concatenation in Localization.lang would use Localization.lang(var + "test") and Localization.lang(var1 + "test" + var2)

Describe the solution you'd like

  • one of these solution should work, not all of them.
  1. finish the ToDo, to cover future string concatenation case in Localization.lang by add StringUtils.countMatches(e.getKey(), "\"") >= 2
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());
  1. just fix comment without code change, such as delete the ToDo comment and add the link to https://devdocs.jabref.org/code-howtos/localization.html

  2. fix the code by delete e.getKey().endsWith("\"")

assertTrue(e.getKey().startsWith("\""), "Illegal localization parameter found. Must include a String with potential concatenation or replacement parameters. Illegal parameter: Localization.lang(" + e.getKey());
@koppor
Copy link
Member

koppor commented Sep 18, 2024

1. finish the ToDo, to cover future string concatenation case in `Localization.lang` by add `StringUtils.countMatches(e.getKey(), "\"") >= 2`

It needs to be ==.

We just support Localization.lang("fixed-string");.

That should be checked. Nothing more.

StringUtils.countMatches(e.getKey() is a great idea 👍. Simple and efficient.


Note that I tried to use Java Parser, but I failed. #8050 (comment)

The other solution for localization is to use Kilt.

However, this does NOT solve the issue of string concatenation. We also do not want that concatenation, because we want to have the strings literally in the source code. Paramters by %0 etc.

We also do NOT want to switch to the syntax of MessageFormat

You can write an ADR if you want 😅

@koppor
Copy link
Member

koppor commented Sep 23, 2024

We worked in the DevCall to a more refined comment. Hope, that helps.

Implemented at #11820

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants