Fix localization task hints #2031

chriba · 2016-09-22T10:25:56Z

fixes: #2029

Replaces hints to the old gradle localization task names with the new ones.

matthiasgeiger

see comments

matthiasgeiger · 2016-09-22T12:30:17Z

src/test/java/net/sf/jabref/logic/l10n/LocalizationConsistencyTest.java

@@ -156,7 +156,7 @@ public void findObsoleteLocalizationKeys() throws IOException {
            System.out.println();
            System.out.println("1. CAREFULLY CHECK IF THE KEY IS REALLY NOT USED ANYMORE");
            System.out.println("2. REMOVE THESE FROM THE ENGLISH LANGUAGE FILE");
-            System.out.println("3. EXECUTE gradlew -b localization.gradle generateMissingTranslationKeys TO");
+            System.out.println("3. EXECUTE gradlew update");


This should be gradlew -b localization.gradle update, right?

Nope, localization.gradle is imported in the build.gradle file, thus gradle knows the tasks form localization.gradle too.

matthiasgeiger · 2016-09-22T12:30:39Z

src/test/java/net/sf/jabref/logic/l10n/LocalizationConsistencyTest.java

@@ -172,7 +172,7 @@ public void findObsoleteMenuLocalizationKeys() throws IOException {
            System.out.println(obsoleteKeys.stream().map(Object::toString).collect(Collectors.joining("\n")));
            System.out.println();
            System.out.println("1. REMOVE THESE FROM THE ENGLISH LANGUAGE FILE");
-            System.out.println("2. EXECUTE gradlew -b localization.gradle generateMissingTranslationKeys" + " TO");
+            System.out.println("3. EXECUTE gradlew update");


same as above

see above, but fixed wrong item enumeration.

matthiasgeiger · 2016-09-22T12:30:45Z

src/test/java/net/sf/jabref/logic/l10n/LocalizationConsistencyTest.java

@@ -197,7 +198,7 @@ private String convertToEnglishPropertiesFile(List<LocalizationEntry> missingKey
    }

    private String convertPropertiesFile(List<LocalizationEntry> missingKeys) {
-        System.out.println("EXECUTE gradlew -b localization.gradle generateMissingTranslationKeys TO");
+        System.out.println("EXECUTE gradlew update");


same as above

stefan-kolb · 2016-09-22T15:36:07Z

CONTRIBUTING.md

@@ -85,7 +85,7 @@ because <additional rationale>.
 Add new Localization.lang("KEY") to Java file.
 Tests fail. In the test output a snippet is generated which must be added to the English translation file. There is also a snippet generated for the non-English files, but this is irrelevant.
 Add snippet to English translation file located at `src/main/resources/l10n/JabRef_en.properties`
-With `gradlew generateMissingTranslationKeys` the "KEY" is added to the other translation files as well.
+With `gradlew update` the "KEY" is added to the other translation files as well.


Hm, I don't like the current task name. It should say what it does, like updateLocalization. Please also change the other Gradle tasks accordingly.

You're right. I use the gradle tasks out of IntelliJ IDEA which groups them by its category.
I added localization as a prefix to each of them.

I also use them that way, thats the reason why I only realized this problem when looking at the PR code.

tobiasdiez · 2016-09-22T21:04:06Z

src/test/java/net/sf/jabref/logic/l10n/LocalizationConsistencyTest.java

@@ -172,7 +172,7 @@ public void findObsoleteMenuLocalizationKeys() throws IOException {
            System.out.println(obsoleteKeys.stream().map(Object::toString).collect(Collectors.joining("\n")));
            System.out.println();
            System.out.println("1. REMOVE THESE FROM THE ENGLISH LANGUAGE FILE");
-            System.out.println("3. EXECUTE gradlew update");
+            System.out.println("2. EXECUTE gradlew localizationUpdate");


If you already touch the localization tests, could you also remove the System.out.println statements by moving everything to the error message.
So for example this here should be
assertEquals("Obsolte kyes found in menu properties file. \n 1. Remove these... \n 2...", emptyList, obsolteKeys)

tobiasdiez

Looks good and can be merged. Just fix the emptyList thing before.

tobiasdiez · 2016-09-23T07:23:17Z

src/test/java/net/sf/jabref/logic/l10n/LocalizationConsistencyTest.java

+                missingKeys.parallelStream()
+                        .map(key -> String.format("%s=%s", key.getKey(), key.getKey()))
+                        .collect(Collectors.toList()),
+                Collections.EMPTY_LIST, missingKeys);


Collections.emptyList() is the preferred way to access it (don't ask me why).

~~The advantage of Collections.EMPTY_LIST is that this list is immutable.~~
Collections.emptyList() is also immutable.

Collections.emptyList() is better because it is a generic implementatin! The constant is not generic

And it's also immuntable: https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#emptyList--
(Unlike this method, the field does not provide type safety.)

Thanks, I changed the lists.

Siedlerchr · 2016-09-24T19:14:57Z

src/test/java/net/sf/jabref/logic/l10n/LocalizationConsistencyTest.java

@@ -128,7 +128,7 @@ public void findMissingLocalizationKeys() throws IOException {
                missingKeys.parallelStream()
                        .map(key -> String.format("%s=%s", key.getKey(), key.getKey()))
                        .collect(Collectors.toList()),
-                Collections.EMPTY_LIST, missingKeys);
+                Collections.<LocalizationEntry>emptyList(), missingKeys);


Just for the future, you could have just taken the Collections.emptyList() without specifying the type explicit.

Siedlerchr

Okay, now looks good!
Failing test is not related

* upstream/master: Implemented Integrity NoBibtexFieldChecker (#2059) Implemented title and camel key modifiers (#1894) Fix localization task hints (#2031) Result of syncLang.py update Result of syncLang.py update (with manual correction of captial_letters, and The_BibTeX_entry...) Fix "large capitals" to "capital letters" Updated Menu_tr.properties (#2057) Updated jabref_tr.properties (#2056) fix ignore version (#2055) Towards hierarchical keywords (#1950) Fix failing test, replace \uxxx encoded chars with proper UTF8 chars. Import Italian patch

* fixLocalizationTaskHint * added prefix to each of the gradle task names * replace system.out with asserts in localization tests

fixLocalizationTaskHint

0857209

boceckts added stupro labels Sep 22, 2016

boceckts approved these changes Sep 22, 2016

View reviewed changes

boceckts added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Sep 22, 2016

matthiasgeiger suggested changes Sep 22, 2016

View reviewed changes

stefan-kolb requested changes Sep 22, 2016

View reviewed changes

added prefix to each of the gradle task names

073dcda

stefan-kolb approved these changes Sep 22, 2016

View reviewed changes

tobiasdiez reviewed Sep 22, 2016

View reviewed changes

replace system.out with asserts in localization tests

978cd4f

tobiasdiez requested changes Sep 23, 2016

View reviewed changes

use generic empty lists

b1c17ec

Siedlerchr reviewed Sep 24, 2016

View reviewed changes

Siedlerchr approved these changes Sep 24, 2016

View reviewed changes

koppor merged commit 3481130 into JabRef:master Sep 25, 2016

koppor mentioned this pull request Sep 25, 2016

Paste localization strings fails #2058

Closed

chriba deleted the fixLocalizationTaskHint branch September 25, 2016 10:01

zesaro pushed a commit to zesaro/jabref that referenced this pull request Nov 22, 2016

Fix localization task hints (JabRef#2031)

ace0b0f

* fixLocalizationTaskHint * added prefix to each of the gradle task names * replace system.out with asserts in localization tests

koppor mentioned this pull request Mar 22, 2018

Sorted language keys alphabetically #3873

Closed

Uh oh!

Fix localization task hints #2031

Fix localization task hints #2031

Uh oh!

Conversation

chriba commented Sep 22, 2016

Uh oh!

matthiasgeiger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriba Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriba Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chriba Sep 22, 2016 •

edited

Loading

chriba Sep 23, 2016 •

edited

Loading