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

Trim whitespaces at the end of line in es_ES Messages.properties file (JENKINS-58842) workaround #4145

Closed
wants to merge 1 commit into from

Conversation

diegombeltran
Copy link

Trim whitespaces at the end of line.

See JENKINS-58842.

Proposed changelog entries

  • Entry 1: Trim whitespaces at the end of line in es_ES Messages.properties file

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Trim whitespaces at the end of line
@oleg-nenashev
Copy link
Member

The PR title does not represent what the PR really does. It makes sense to trim spaces, but it has nothing to do with IAM roles

@MRamonLeon
Copy link
Contributor

Hello @diegombeltran . First of all, thank you for your contribution, appreciated. Welcome to the Jenkins community.

I see a lot of changes in the diff that effectively doesn't change anything. Could you please remove these changes from the PR to leave it cleaner? And change the title as Oleg suggested?
Example:
The text is the same as before:
ResultTrend.StillFailing=Todavía falla -> ResultTrend.StillFailing=Todavía falla

Have you checked that the messages in other languages (English for example) don't have these blanks at the end? Maybe the message is concatenated to another one, so the blank at the end is needed.

Gracias!

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jenkinsci/aws-credentials-plugin#69 for a proper fix of the issue. I do not mind merging this pull request as well since it really cleans up the localization. But it is definitely not a root cause of JENKINS-58842

@oleg-nenashev oleg-nenashev changed the title [JENKINS-58842] Error assuming IAM role in Jenkins (es_ES) Trim whitespaces at the end of line in es_ES Messages.properties file (JENKINS-58842) workaround Aug 16, 2019
@@ -120,11 +120,11 @@ Job.NOfMFailed={0} de las {1} \u00faltimas ejecuciones fallaron.
Job.NoRecentBuildFailed=No hay ejecuciones recientes con fallos.
Job.Pronoun=Proyecto
Job.minutes=Min
Job.NoRenameWhileBuilding=No es posible renombrar un proyecto mientras se est� ejecutando.
Job.NoRenameWhileBuilding=No es posible renombrar un proyecto mientras se está ejecutando.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub refuses to show a diff here, but it seems you changed the encoding to UTF-8. It must be ISO-8859-1. Acc. to jshell,

var p = new Properties()
p.load(new java.io.FileInputStream("/…/jenkinsci/jenkins/core/src/main/resources/hudson/model/Messages_es.properties"))
p.getProperty("Job.NoRenameWhileBuilding")

returns

No es posible renombrar un proyecto mientras se está ejecutando.

while the equivalent passing in the file as patched here produces mojibake

No es posible renombrar un proyecto mientras se está ejecutando.

Safest to throw away this modification and start over with an editor or tool that can safely handle ISO-8859-1 text.

@jglick
Copy link
Member

jglick commented Aug 16, 2019

Note that Java supports loading *.properties in an encoding of your choice, but this is worse than nothing IMO because it does so based on a parameter passed by the Java code loading the file, not by inspecting the file for some magic header or BOM or something that could also be interpreted by editors and other tooling; so editors must assume that the file uses the historical default of ISO-8859-1. For example, the NetBeans editor always loads *.properties in this encoding, regardless of project-wide defaults.

Perhaps we should deprecate use of java.util.Properties for Messages classes (and other usages, like Stapler localization of *.jelly) altogether, and switch to some file format more familiar to the youth of today which is defined to be UTF-8.

@MRamonLeon
Copy link
Contributor

I see a lot of changes in the diff that effectively doesn't change anything. Could you please remove these changes from the PR to leave it cleaner?

That should be the problem, GitHub is writing the same text, no matter which encoding the file has. Please double-check the encoding of the file.

@diegombeltran
Copy link
Author

Hello,

First of all thank you for spending time on this. I'm currently on my holidays far from computers, I will check this on Saturday or so.

See you!

@jglick jglick added the work-in-progress The PR is under active development, not ready to the final review label Aug 19, 2019
@jglick
Copy link
Member

jglick commented Sep 23, 2019

deprecate use of java.util.Properties for Messages classes (and other usages

See kohsuke/localizer#20 for example. CC @ikedam

@varyvol
Copy link

varyvol commented Mar 19, 2020

@diegombeltran did you eventually have the opportunity to spend some time on this? I'm going to propose it for close but please feel free to reach in case you think otherwise.

@varyvol varyvol added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Mar 19, 2020
@rsandell rsandell closed this Mar 26, 2020
@rsandell
Copy link
Member

No response from author, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it work-in-progress The PR is under active development, not ready to the final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants