8295729: Add jcheck whitespace checking for properties files #10792
Conversation
|
👋 Welcome back ihse! A progress list of the required criteria for merging this PR into |
|
@magicus The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
javadoc
Webrevs
|
|
@magicus This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 116 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
I would vote against this change. Per java properties spec This change might break localization or messages where trailing whitespace is important (example: "Label: ") edit: sorry, the link above is not a spec. looking at the Properties.load(Reader) javadoc: |
|
@andy-goryachev-oracle Oh, I did not know that. Is this really how it is implemented, or is there a discrepancy between the spec and the implementation? The haphazard placement of trailing spaces seems to indicate that they are ignored. |
Although I didn't know this was in the spec, I suspected it might be the case. When looking at the jdk.management.agent changes, it looked like the extra space was actually a typo and was copied into all the localized versions (and not actually localized for unicode locales). In this case removing the white space is the right thing to do. It is in fact fixing an actual bug. |
|
@magicus : outputs: |
|
Okay. That definitely rules out adding .properties to the whitespace jcheck verification. However, I think that instead opens up a ton of more individual problems, since I think most (if perhaps not all) of these trailing whitespaces are incidental, and thus might be bugs. Perhaps no-one really noticed a double whitespace where it were not supposed to be, etc, especially not if it was just for a translated language. I'm thinking I should redirect this PR to skip the jcheck requirement, and revert all changes to property values, but keep the part of the patch that removes trailing spaces in For the remaining properties files with trailing spaces in the actual values, I'll make a sanity check if it seems correct or not, and if it looks fishy, I'll open bugs on the respective component teams to verify that their property values are indeed correct. Does that sound reasonable? |
|
Retracting my approval too. Thanks for the catch, Andy! |
magicus
changed the title
|
368 files to review! I wish we had separate PRs for fixing properties and checking for trailing whitespace. i 'd expect it will take some time to review localization, unless we just keep them as is. |
|
I agree, that was an excellent idea Naoto! In my last commit, I have converted all trailing spaces/tabs in value lines into explicit unicode characters. Since that means we have no trailing spaces (from a jcheck perspective), I could also restore the jcheck that was the original driver behind this bug. (And also restore the bug/PR title to show the original intent.) |
|
@andy-goryachev-oracle They are all automatically processed. There are two kinds of changes: Non-value lines have their trailing whitespace removed. You can verify that part of the PR by looking here: https://github.com/openjdk/jdk/pull/10792/files/c91fdaa19dc06351598bd1c0614e1af3bfa08ae2 This was the state of the PR as of three days ago. The most numerous number of changed files are here, but you can just scroll through the change and verify quickly that it is trivial. The second change is the one suggested by Naoti; I have replaced all trailing tabs (there were just one) with This affects far fewer files. And once again, it was mechanically generated. You can once again just scroll through and verify that all changes are due to the trailing whitespace being converted to unicode sequences. |
|
For the files which have trailing "whitespace" (now as unicode sequences), I will file follow-up bugs on the respective components to verify if this is indeed correct, or a bug that should be fixed. I did not think it was a good idea to hold this PR, waiting for component teams to do the whitespace check first, for two reasons:
|
|
And finally: Here is a complete list of the files which has trailing "unicode whitespace" in values. I will try to figure out to which components these belongs and open corresponding bugs. |
| @@ -184,7 +184,7 @@ MultiMenu.single_accessible_description=SwingSet2\u306E\u30A6\u30A3\u30F3\u30C9\ | |||
|
|
|||
| ### Button Demo ### | |||
|
|
|||
| ButtonDemo.accessible_description=ButtonDemo\u3067\u306F\u3001JButton\u3001JRadioButton\u3001JToggleButton\u304A\u3088\u3073JCheckBox\u306E\u4F7F\u7528\u4F8B\u3092\u7D39\u4ECB\u3057\u307E\u3059 | |||
| ButtonDemo.accessible_description=ButtonDemo\u3067\u306F\u3001JButton\u3001JRadioButton\u3001JToggleButton\u304A\u3088\u3073JCheckBox\u306E\u4F7F\u7528\u4F8B\u3092\u7D39\u4ECB\u3057\u307E\u3059\u0020 | |||
There was a problem hiding this comment.
trailing whitespace looks unnecessary (accessible description?)
There was a problem hiding this comment.
although this is in demo...
| @@ -64,7 +64,7 @@ FileChooser.newFolderParentDoesntExistTitle.textAndMnemonic=Ordner kann nicht er | |||
| FileChooser.newFolderParentDoesntExist.textAndMnemonic=Ordner kann nicht erstellt werden.\n\nSystem kann den angegebenen Pfad nicht finden. | |||
| FileChooser.renameErrorTitle.textAndMnemonic=Fehler beim Umbenennen von Datei oder Ordner | |||
| FileChooser.renameError.textAndMnemonic={0} kann nicht umbenannt werden | |||
| FileChooser.renameErrorFileExists.textAndMnemonic={0} kann nicht umbenannt werden: Es ist bereits eine Datei mit dem angegebenen Namen vorhanden. Geben Sie einen anderen Dateinamen an. | |||
| FileChooser.renameErrorFileExists.textAndMnemonic={0} kann nicht umbenannt werden: Es ist bereits eine Datei mit dem angegebenen Namen vorhanden. Geben Sie einen anderen Dateinamen an.\u0020 | |||
| @@ -64,7 +64,7 @@ FileChooser.newFolderParentDoesntExistTitle.textAndMnemonic=No se ha podido crea | |||
| FileChooser.newFolderParentDoesntExist.textAndMnemonic=No se ha podido crear la carpeta.\n\nEl sistema no puede encontrar la ruta de acceso especificada. | |||
| FileChooser.renameErrorTitle.textAndMnemonic=Error al cambiar el nombre del archivo o carpeta | |||
| FileChooser.renameError.textAndMnemonic=No se puede cambiar el nombre de {0} | |||
| FileChooser.renameErrorFileExists.textAndMnemonic=No se puede cambiar el nombre de {0}: ya existe un archivo con el nombre especificado. Especifique otro nombre de archivo. | |||
| FileChooser.renameErrorFileExists.textAndMnemonic=No se puede cambiar el nombre de {0}: ya existe un archivo con el nombre especificado. Especifique otro nombre de archivo.\u0020 | |||
| @@ -64,7 +64,7 @@ FileChooser.newFolderParentDoesntExistTitle.textAndMnemonic=Impossible de cr\u00 | |||
| FileChooser.newFolderParentDoesntExist.textAndMnemonic=Impossible de cr\u00E9er le dossier.\n\nLe syst\u00E8me ne parvient pas \u00E0 trouver le chemin indiqu\u00E9. | |||
| FileChooser.renameErrorTitle.textAndMnemonic=Erreur lors du changement de nom du fichier ou du dossier | |||
| FileChooser.renameError.textAndMnemonic=Impossible de renommer {0} | |||
| FileChooser.renameErrorFileExists.textAndMnemonic=Impossible de renommer {0} : il existe d\u00E9j\u00E0 un fichier portant le nom indiqu\u00E9. Indiquez-en un autre. | |||
| FileChooser.renameErrorFileExists.textAndMnemonic=Impossible de renommer {0} : il existe d\u00E9j\u00E0 un fichier portant le nom indiqu\u00E9. Indiquez-en un autre.\u0020 | |||
| @@ -64,7 +64,7 @@ FileChooser.newFolderParentDoesntExistTitle.textAndMnemonic=Impossibile creare l | |||
| FileChooser.newFolderParentDoesntExist.textAndMnemonic=Impossibile creare la cartella.\n\nIl sistema non \u00E8 in grado di trovare il percorso specificato. | |||
| FileChooser.renameErrorTitle.textAndMnemonic=Errore durante la ridenominazione del file o della cartella | |||
| FileChooser.renameError.textAndMnemonic=Impossibile rinominare {0} | |||
| FileChooser.renameErrorFileExists.textAndMnemonic=Impossibile rinominare {0}: esiste gi\u00E0 un file con il nome specificato. Specificare un altro nome. | |||
| FileChooser.renameErrorFileExists.textAndMnemonic=Impossibile rinominare {0}: esiste gi\u00E0 un file con il nome specificato. Specificare un altro nome.\u0020 | |||
| crswriter.conflictsno = conflicts while synchronizing | ||
| crswriter.params1 = Value of params1 : {0}\u0020 | ||
| crswriter.params2 = Value of params2 : {0}\u0020 | ||
| crswriter.conflictsno = conflicts while synchronizing\u0020 |
There was a problem hiding this comment.
this is tricky. if this value is a part of a sentence (i.e. something like "5 conflicts..."), the localization is likely to be wrong. it's hard to tell without looking further into the code.
| @@ -41,7 +41,7 @@ cachedrowsetimpl.intfail = getInt bei Wert ( {0} ) in Spalte {1} nicht erfolgrei | |||
| cachedrowsetimpl.longfail = getLong bei Wert ( {0} ) in Spalte {1} nicht erfolgreich | |||
| cachedrowsetimpl.floatfail = getFloat bei Wert ( {0} ) in Spalte {1} nicht erfolgreich | |||
| cachedrowsetimpl.doublefail = getDouble bei Wert ( {0} ) in Spalte {1} nicht erfolgreich | |||
| cachedrowsetimpl.dtypemismt = Keine Datentyp\u00FCbereinstimmung | |||
| cachedrowsetimpl.dtypemismt = Keine Datentyp\u00FCbereinstimmung\u0020 | |||
| @@ -84,7 +84,7 @@ webrowsetimpl.invalidwr = Ung\u00FCltiger Writer | |||
| webrowsetimpl.invalidrd = Ung\u00FCltiger Reader | |||
|
|
|||
| #FilteredRowSetImpl exceptions | |||
| filteredrowsetimpl.relative = relative: Ung\u00FCltiger Cursorvorgang | |||
| filteredrowsetimpl.relative = relative: Ung\u00FCltiger Cursorvorgang\u0020 | |||
| @@ -70,7 +70,7 @@ cachedrowsetimpl.unsetmatch1 = Spaltenname als Argument f\u00FCr unsetMatchColum | |||
| cachedrowsetimpl.unsetmatch2 = Spalten-ID als Argument f\u00FCr unsetMatchColumn verwenden | |||
| cachedrowsetimpl.numrows = Zeilenanzahl ist kleiner als null oder kleiner als Abrufgr\u00F6\u00DFe | |||
| cachedrowsetimpl.startpos = Startposition darf keinen Negativwert aufweisen | |||
| cachedrowsetimpl.nextpage = Daten m\u00FCssen vor dem Aufruf ausgef\u00FCllt werden | |||
| cachedrowsetimpl.nextpage = Daten m\u00FCssen vor dem Aufruf ausgef\u00FCllt werden\u0020 | |||
|
|
||
| #InsertRow exceptions | ||
| insertrow.novalue = Es wurde kein Wert eingef\u00FCgt | ||
|
|
||
| #SyncResolverImpl exceptions | ||
| syncrsimpl.indexval = Indexwert liegt au\u00DFerhalb des Bereichs | ||
| syncrsimpl.indexval = Indexwert liegt au\u00DFerhalb des Bereichs\u0020\u0020 |
|
I think it would be better to try and remove incidental trailing whitespace first, before encoding any remaining whitespace. Hiding the trailing whitespace as a Unicode escape seems like a bad idea, equivalent to sweeping the issue under the rug. While I agree with the goals of improving the check, I think this is going about it the wrong way, or at least in the wrong order. Maybe it would be a good idea to first validate the default/English files, checking for incidental whitespace there, then check localized versions of each property against the English version. |
probably not needed - if nobody noticed anything until now we have no problem. the solution to escape whitespace in values is the right solution, solves both the jcheck and WS visibility issues. good job! (and we can ignore my "prob. unnecessary" comments) |
|
The problem here is that all those (unnecessary) trailing spaces are appended by the external translators, who are not aware those spaces should not be at the end. I think what we can do is check the original English properties values that the engineers provided, and if there is no trailing spaces there, we can safely remove trailing spaces in localized bundles. |
Good idea! I wonder if this should be done as a unit test. go through all the bundles and check leading/trailing whitespace. in my experience, the translators also (unintentionally) change the quotes and other symbols, sometimes breaking the code. I assume the JDK has been exhaustively tested and we have no such problems. |
Right. Definitely not a job for |
I respectfully disagree. I think changing a trailing " " into a "\u0020" is the opposite of hiding it; it is making it plainly visible. In fact, I believe most of these trailing spaces are the result of them not being visible enough (and the tooling not warning). Secondly, there are a lot of definitely unintentional trailing spaces, in comments and blank lines. I'd say there is factor 10:1 more of these. Getting these out of the way allows developers to focus more clearly on the trailing whitespace that matters: those in the key-value pairs. And as I said, I intend to file follow-up issues for all files where there is a trailing unicode-sequence whitespace, so it will definitely not be lost on the respective component teams that they have something they need to address.
That's probably a good idea, but I think we should leave that to each respective team. Just like Andy's and Naoto's suggestion of improving i18n tooling to detect issues like this earlier on. Good idea, but I'd like to see that implemented separated from this PR. This PR is already large. The only reason it makes sense is because all changes (except the one to jcheck) are automatically generated and trivial to verify correctness of. If we were to start adding individual, manual changes into this PR, it would be just a huge mess. |
| @@ -24,7 +24,7 @@ | |||
| # | |||
|
|
|||
| agent.err.error = Error | |||
| agent.err.exception = Exception thrown by the agent | |||
| agent.err.exception = Exception thrown by the agent\u0020 | |||
There was a problem hiding this comment.
I believe this space was just a typo and should be removed. Same for agent.err.agentclass.failed below and in all the other management property files.
| @@ -2,4 +2,4 @@ modules = \ | |||
| java.base/sun.security.provider.certpath \ | |||
| java.base/sun.security.util \ | |||
| java.base/sun.security.validator \ | |||
| java.base/sun.security.x509 | |||
| java.base/sun.security.x509\u0020 | |||
There was a problem hiding this comment.
I'm quite sure this space can be safely removed.
|
I noticed another problem. In some English properties files (Ex: I suggest we focus on the English files this time and file a bug to the translation tool. |
Good catch, Max. Yes, that should be dealt with in the translation process.
Agree. |
There was a problem hiding this comment.
Trailing spaces in LocaleNames_* are only in two files:
src/jdk.localedata/share/classes/sun/util/resources/ext/LocaleNames_de.propertiessrc/jdk.localedata/share/classes/sun/util/resources/ext/LocaleNames_sv.properties
It is very unlikely these spaces are part of a country or language name. The former file contains a few trailing spaces, the latter — only one.
| @@ -235,7 +235,7 @@ cpe=Kreolisch-Englische Sprache | |||
| cpf=Kreolisch-Franz\u00f6sische Sprache | |||
| cpp=Kreolisch-Portugiesische Sprache | |||
| crh=Krimtatarisch | |||
| crp=Kreolische Sprache | |||
| crp=Kreolische Sprache\u0020 | |||
There was a problem hiding this comment.
I'm pretty sure locale names shouldn't contain trailing spaces.
|
@magicus This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
Yes bot, I know. I'll need to split this up into multiple steps, but please keep this open for a while more. Thank you. |
|
This turned out to be much more complicated than anticipated. I am going to close this PR (and bug), and instead I have split up this work in five different bugs: JDK-8298047 is about removing all non-significant whitespace from all properties files, basically corresponding to how this PR looked after the second commit. JDK-8298042, JDK-8298044, JDK-8298045 and JDK-8298046 is about fixing the significant whitespace, by turning it to unicode sequences or removing it, if it turns out there should not have been a space there. This work is split into four parts to be able to have one bug per component team. |
would you consider also adding a unit test to check whether the localized resources also contain leading/trailing whitespace, and possibly special characters (like {, }, , etc.) that are present in the main resource? |
|
@andy-goryachev-oracle No. Any such test is up to the component owners to write, if they deem it fitting. |
Properties files is essentially source code. It should have the same whitespace checks as all other source code, so we don't get spurious trailing whitespace changes.
With the new Skara jcheck, it is possible to increase the coverage of the whitespace checks (in the old mercurial version, this was more or less impossible).
The only manual change is to
.jcheck/conf. All other changes were made by runningfind . -type f -iname "*.properties" | xargs gsed -i -e 's/[ \t]*$//'.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10792/head:pull/10792$ git checkout pull/10792Update a local copy of the PR:
$ git checkout pull/10792$ git pull https://git.openjdk.org/jdk pull/10792/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10792View PR using the GUI difftool:
$ git pr show -t 10792Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10792.diff