localisationTestForInvalidStrings #3198

steph37tours · 2017-09-05T13:44:52Z

…te for instance

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

…te for instance

LinusDietz · 2017-09-05T14:59:07Z

Hey @steph37tours, this goes into the right direction. I think the test fails in the right places, but the message is quite big.
What would be better is something like this:
Found an invalid character in the <language> localization of <JabRef | Menu>: The <key | value> <actual string> contains a space!

This would require to iterate over all property strings, similar as in LocalizationConsistencyTest.keyValueShouldBeEqualForEnglishPropertiesMenu().
Please give it another shot.

steph37tours · 2017-09-05T15:16:04Z

Thank you @Lynyus. I see what you mean. I'll correct this tomorrow if it's ok :-).

Ok, I am on actually.

koppor · 2017-09-05T14:53:08Z

src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java

+                        InputStreamReader reader = new InputStreamReader(is, StandardCharsets.UTF_8)) {
+                    properties.load(reader);
+                } catch (IOException e) {
+                    throw new RuntimeException(e);


No need to catch and throw - the method already throws the exception

koppor · 2017-09-05T16:03:51Z

I am unsure whether JabRef really cannot handle underscores. I once looked at the code and I think, we could get rid of using the underscores and always use spaces. Could you check that? I think _ is so 2005 😇

LinusDietz · 2017-09-05T16:11:58Z

Wow @koppor (;

koppor

Exceptions typically don't need to be catched in JUnit test cases

LinusDietz · 2017-09-06T08:44:24Z

@koppor to make this crystal clear: Do we need to fix the underscores or not?
In

jabref/src/main/java/org/jabref/logic/l10n/LocalizationKey.java

Line 14 in ebbeb1d

// space, #, !, = and : are not allowed in properties file keys

there is a comment that fixes the spaces, but not the #, !, =, and :.
So is there need to pursue this PR, or not?

Siedlerchr · 2017-09-06T09:00:09Z

@koppor @lynyus That is specified in the java docs:
http://docs.oracle.com/javase/8/docs/api/java/util/Properties.html#load(java.io.Reader)

.A comment line has an ASCII '#' or '!' as its first non-white space character; comment lines are also ignored and do not encode key-element information
In addition to line terminators, this format considers the characters space (' ', '\u0020'), tab ('\t', '\u0009'), and form feed ('\f', '\u000C') to be white space.

The key contains all of the characters in the line starting with the first non-white space character and up to, but not including, the first unescaped '=', ':', or white space character other than a line terminator

koppor · 2017-09-06T09:21:08Z

Then, we can have the values without underscores? That would improve the language files a lot!

LinusDietz · 2017-09-06T09:23:07Z

The question is, of course, why they were introduced in the first place. Is there any pitfall if we remove them?!?

Siedlerchr · 2017-09-06T09:33:33Z

From what I understand the key can't contain whitespaces or the first whitepsace would be recognized as spearator in the key:
A key is all the characters up to the first whitespace or a key/value separator
http://kajabity.com/kajabity-tools/java-properties-classes/the-java-properties-file-format/

there are spaces in the localization values and show detailed error.

steph37tours · 2017-09-06T10:00:48Z

I just push a change. I hope it is what you want. I've got difficult to test it.
And I'm not sure for the "throws IOException" Eclipse ask me for remove it.
Thank you for help and permit to participate. It was very interesting for me.

LinusDietz

The functionality is not yet doing what it should. Please go through the comments and try to run the tests yourself.

In order to merge this pull request it would also be necessary to fix the invalid localization files such that the test actually passes. If you want to, I can help you with that. In this case you would have to give me push access to your fork of JabRef.

LinusDietz · 2017-09-06T10:27:02Z

src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java

@@ -183,6 +183,28 @@ public void localizationParameterMustIncludeAString() throws IOException {
        }
    }

+    @Test
+    public void localisationTestForInvalidStrings() throws IOException {


Indeed, you can remove the throws statement.

LinusDietz · 2017-09-06T10:28:10Z

src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java

+                for (Map.Entry<Object, Object> entry : textKeys.entrySet()) {
+                    String expectedKeyEqualsKey = String.format("%s=%s", entry.getKey(), entry.getKey());
+                    String actualKeyEqualsValue = String.format("%s=%s", entry.getKey(),
+                            entry.getValue().toString().replace(" ", ""));


I would suggest that you check for spaces using String.contains(). Then there is no need to compare strings. later.

LinusDietz · 2017-09-06T10:29:47Z

src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java

+                //parse object "textKeys" to find any spaces
+                for (Map.Entry<Object, Object> entry : textKeys.entrySet()) {
+                    String expectedKeyEqualsKey = String.format("%s=%s", entry.getKey(), entry.getKey());
+                    String actualKeyEqualsValue = String.format("%s=%s", entry.getKey(),


This is not correct. While in the english localization the key must equal the value, in all other languages the the key is different from the value.

LinusDietz · 2017-09-06T10:34:53Z

src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java

+                            entry.getValue().toString().replace(" ", ""));
+
+
+                    assertEquals("Found an invalid character in the "+ lang + "localization of" + bundle + ": The " + entry.getKey().toString() + ":" + expectedKeyEqualsKey + " contains a space!",expectedKeyEqualsKey, actualKeyEqualsValue);


This looks fine, however make sure to include some spaces and quotes to make the sentence more readable
Found an invalid character in the inlocalization ofJabRef: The pairs_processed:pairs_processed=pairs_processed contains a space!
=>
Found an invalid character in the 'in' localization of 'JabRef': The 'pairs_processed:pairs_processed=pairs_processed' contains a space!

Also the error should be apparent from the message. The String above does not have a space.

I give you push access. I'm correcting my code.

I think it's ok now. But I didn't success to test ! It seems I don't know how to do.

LinusDietz

We are getting there soon. Check my remarks!

Try to run the tests using eclipse: It's described here https://stackoverflow.com/questions/646307/running-a-single-junit-test-in-eclipse how to do it.

As soon as the test is fine, I'll fix the files and we are good to merge.

LinusDietz · 2017-09-06T11:57:40Z

src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java

+                        .getProperties(propertyFilePath);
+                //parse object "textKeys" to find any spaces
+                for (Map.Entry<Object, Object> entry : textKeys.entrySet()) {
+                    assertTrue("Found an invalid character in the " + lang + " localization of " + bundle + " : The " + entry.getKey().toString() + " : " + entry.getValue().toString() + " contains a space!", entry.getValue().toString().contains(" "));


The statement must be inverted. We want the test to fail if there is a ' ' in the String.
Also, you only verify the values, but not the keys of the entry.
Just add another line where you assert that there are also no spaces in the key.

@lynyus, I commit and push again.
I correct the message with "At key", which seems to me more clear to find the line.
For the test : I use Junit. It find some failures

java.lang.AssertionError: Found an invalid character in the in localization of JabRef : proces_berpasangan At key : pairs_processed contains a space!

If I correct the P into Pairs_processed, it runs.

But I've got the same trouble in New_article , but I don't find why...

Have you tried using assertFalse instead if assertTrue?

Yes. It runs !

LinusDietz · 2017-09-06T11:59:21Z

src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java

@@ -183,6 +183,23 @@ public void localizationParameterMustIncludeAString() throws IOException {
        }
    }

+    @Test
+    public void localisationTestForInvalidStrings() {


I would suggest to make this name a bit more precise: maybe localizationTestForInvalidCharacters?

-correct name change s with z -change messages more clear

LinusDietz · 2017-09-06T13:33:56Z

So if you change the Assertions to assertFalse or negate the condition the tests should be operational and we can see if they pass the test on our build server
assertFalse("Found an invalid character in the " + lang + " localization of " + bundle + " : " + entry.getValue().toString() + " At key : " + entry.getKey().toString() + " contains a space!", entry.getValue().toString().contains(" "));

steph37tours · 2017-09-06T13:36:48Z

Yes, I change and commit. I found spaces in pt_BR localization :

java.lang.AssertionError: Found an invalid character in the pt_BR localization of JabRef : Normalizar para o formato_de_nome_do_BibTeX At key : Normalize_to_BibTeX_name_format contains a space!

steph37tours · 2017-09-06T13:40:00Z

So is-it done now for me ?

LinusDietz · 2017-09-06T13:41:23Z

Can you push your commit please? (; You'll have to pull first, as I have already fixed the localization files on your branch.

steph37tours · 2017-09-06T13:54:35Z

sorry, it doesn't want to push ! It seems I have to pull your change before. I 'm looking at...

steph37tours · 2017-09-06T14:13:00Z

I'm sorry I can't push !
I've got the error that the branch countain work I doesn't have in local.
I try git pull
git pull --rebase without success for instance !

I'm looking for always. You can help !

LinusDietz · 2017-09-06T14:14:47Z

on the current branch try the following:
git fetch
git merge
git push

steph37tours · 2017-09-06T14:22:11Z

git merge said not remote branch for the current

I'm sorry to be so beginner...

steph37tours · 2017-09-06T14:28:26Z

git merge Localisation_test_for_invalid_Strings
said up-to-date

LinusDietz · 2017-09-06T14:28:59Z

Hm. Maybe try a GUI client like Github Desktop to figure everything out. Or clone the repository again and apply your changes again.

steph37tours · 2017-09-06T15:17:10Z

I'm sorry, I just clone and try to push again with the same error message. I continue to look for a solution.

This reverts commit ffa51c6.

steph37tours · 2017-09-06T15:54:01Z

I don't know what to do.
Into Eclipse, the menu team/pull give to me an error "Missing commit"

steph37tours · 2017-09-06T15:54:30Z

perhaps I have to do another fork ?

…b.com/steph37tours/jabref into Localisation_test_for_invalid_Strings Follow trouble with pull

steph37tours · 2017-09-06T16:17:32Z

It seems it's ok now. I don't know what happen exactly.
Really thank you for this first experience, thank you for the time you share with me to help.

Siedlerchr · 2017-09-06T16:56:06Z

src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java

+
+                //parse object "textKeys" to find any spaces
+                for (Map.Entry<Object, Object> entry : textKeys.entrySet()) {
+                    assertFalse("Found an invalid character in the " + lang + " localization of " + bundle + " : " + entry.getValue().toString() + " At key : " + entry.getKey().toString() + " contains a space!", entry.getValue().toString().contains(" "));


Just a minor thing: if you concatenate Strings and objects (no matter which type) with"some text " + object
You don't need to do an explicit call to "toString()" The method is automatically invoked. But beware, in the case where you want to call a methond on a string like the contains, you have to use the toString

Thank you @Siedlerchr for this explanation.

LinusDietz · 2017-09-07T09:05:22Z

I'll merge this now, thanks for the efforts, @steph37tours! I hope you have learned something and are still eager to contribute. (;
How about this? #2942 It's a small feature in the UI, and everybody would be quite happy if somebody finds time to implement it!

steph37tours · 2017-09-07T12:25:13Z

Thank you very much @lynyus !
I learn many things with this experience.
So I agree to continue ! :-)

koppor · 2017-09-08T12:37:55Z

Good to see that you won the git war :). -- For learning git, I would recommend the tutorials listed at https://github.com/dictcp/awesome-git.

steph37tours · 2017-09-08T13:31:48Z

Thank you @koppor for the link ! I'll study it !

steph37tours added 2 commits September 5, 2017 15:37

adding of the new method localisationTestForInvalidStrings not comple…

df30b6a

…te for instance

complete of the method localisationTestForInvalidStrings

df53174

steph37tours closed this Sep 5, 2017

steph37tours reopened this Sep 5, 2017

LinusDietz mentioned this pull request Sep 5, 2017

Localisation test for invalid Strings #3185

Closed

LinusDietz changed the title ~~adding of the new method localisationTestForInvalidStrings not comple…~~ [WIP] localisationTestForInvalidStrings Sep 5, 2017

koppor reviewed Sep 5, 2017

View reviewed changes

Modification to the method localisationTestForInvalidStrings to test if

0e4a491

there are spaces in the localization values and show detailed error.

LinusDietz requested changes Sep 6, 2017

View reviewed changes

Correcting the method localisationTestForInvalidStrings

7c43a61

LinusDietz requested changes Sep 6, 2017

View reviewed changes

steph37tours and others added 2 commits September 6, 2017 15:05

Correcting the method localizationTestForInvalidStrings

85d897a

-correct name change s with z -change messages more clear

Fix three invalid localizations

2dd0715

method localizationTestForInvalidStrings complete

ffa51c6

Revert "method localizationTestForInvalidStrings complete"

1d6de83

This reverts commit ffa51c6.

steph37tours added 2 commits September 6, 2017 17:57

Merge branch 'Localisation_test_for_invalid_Strings' of https://githu…

aeac06c

…b.com/steph37tours/jabref into Localisation_test_for_invalid_Strings Follow trouble with pull

method localizationTestForInvalidStrings complete

96d1bf4

Siedlerchr reviewed Sep 6, 2017

View reviewed changes

Finalize the localizationTestForInvalidStrings, improve messages

00b8069

LinusDietz changed the title ~~[WIP] localisationTestForInvalidStrings~~ localisationTestForInvalidStrings Sep 7, 2017

LinusDietz merged commit 529fa43 into JabRef:master Sep 7, 2017

		entry.getValue().toString().replace(" ", ""));


		assertEquals("Found an invalid character in the "+ lang + "localization of" + bundle + ": The " + entry.getKey().toString() + ":" + expectedKeyEqualsKey + " contains a space!",expectedKeyEqualsKey, actualKeyEqualsValue);

localisationTestForInvalidStrings #3198

localisationTestForInvalidStrings #3198

Conversation

steph37tours commented Sep 5, 2017 • edited Loading

LinusDietz commented Sep 5, 2017

steph37tours commented Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

koppor commented Sep 5, 2017

LinusDietz commented Sep 5, 2017

koppor left a comment

Choose a reason for hiding this comment

LinusDietz commented Sep 6, 2017

Siedlerchr commented Sep 6, 2017

koppor commented Sep 6, 2017 • edited Loading

LinusDietz commented Sep 6, 2017

Siedlerchr commented Sep 6, 2017 • edited Loading

steph37tours commented Sep 6, 2017

LinusDietz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinusDietz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinusDietz commented Sep 6, 2017

steph37tours commented Sep 6, 2017

steph37tours commented Sep 6, 2017 • edited Loading

LinusDietz commented Sep 6, 2017

steph37tours commented Sep 6, 2017

steph37tours commented Sep 6, 2017 • edited Loading

LinusDietz commented Sep 6, 2017

steph37tours commented Sep 6, 2017 • edited Loading

steph37tours commented Sep 6, 2017

LinusDietz commented Sep 6, 2017

steph37tours commented Sep 6, 2017

steph37tours commented Sep 6, 2017

steph37tours commented Sep 6, 2017

steph37tours commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinusDietz commented Sep 7, 2017

steph37tours commented Sep 7, 2017 • edited Loading

koppor commented Sep 8, 2017

steph37tours commented Sep 8, 2017

steph37tours commented Sep 5, 2017 •

edited

Loading

steph37tours commented Sep 5, 2017 •

edited

Loading

koppor commented Sep 6, 2017 •

edited

Loading

Siedlerchr commented Sep 6, 2017 •

edited

Loading

LinusDietz left a comment •

edited

Loading

steph37tours commented Sep 6, 2017 •

edited

Loading

steph37tours commented Sep 6, 2017 •

edited

Loading

steph37tours commented Sep 6, 2017 •

edited

Loading

steph37tours commented Sep 7, 2017 •

edited

Loading