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

MessageFormat treats ' as a special character #3203

Merged
merged 1 commit into from Jan 20, 2018

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Dec 19, 2017

https://docs.oracle.com/javase/8/docs/api/java/text/MessageFormat.html

Test case is jenkins/computer/node/configure
Help for "Launch method"

Without this fix, you will see:
Launch agent via Java Web Start
... By default, the JNLP agent will launch a GUI, but its also possible to run a JNLP agent without a GUI, e.g. as a Window service.

You should see:
... By default, the JNLP agent will launch a GUI, but it's also possible to run a JNLP agent without a GUI, e.g. as a Window service.

Submitter checklist

  • 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.)

Desired reviewers

https://docs.oracle.com/javase/8/docs/api/java/text/MessageFormat.html

Test case is jenkins/computer/node/configure
 Help for "Launch method"

Without this fix, you will see:
Launch agent via Java Web Start
... By default, the JNLP agent will launch a GUI, but its also possible to run a JNLP agent without a GUI, e.g. as a Window service.

You should see:
... By default, the JNLP agent will launch a GUI, but it's also possible to run a JNLP agent without a GUI, e.g. as a Window service.
@daniel-beck
Copy link
Member

I think @jglick had somewhat strong feelings that these should just be typographic quotes instead.

@jsoref
Copy link
Contributor Author

jsoref commented Dec 19, 2017

From my perspective, there are potentially two bugs here:

  1. your single quotes need to be doubled to be seen -- my patch does this. This is a strict improvement over what is currently present.
  2. you may want to replace single quotes w/ typographic quotes. @jglick may want that.

At the expense of repo churn, I'd rather get my change in and have someone else consider a more global typographic quotes commit.

@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Dec 22, 2017
@jglick
Copy link
Member

jglick commented Dec 25, 2017

You definitely need to double up ' when there is actually a message format: {0}, etc. But if there are no parameters, as in the example shown, there is no reason to process anything using MessageFormat to begin with—it should have just been treated as a literal string. So I wonder if there is a bug in stapler-jelly at the root of this.

@jsoref
Copy link
Contributor Author

jsoref commented Dec 26, 2017

My guess is that there's only one path, it's designed for localization and is consistent and agnostic.

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

seems reasonable, assuming it was tested.

@jglick
Copy link
Member

jglick commented Jan 8, 2018

Indeed this code shows that Stapler applies MessageFormat even when there are no arguments to format, which is surprising and undesirable, but presumably too late to fix now—if anyone is already using '' inside a zero-argument string, we must continue to render it as '.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Generally prefer use of (or, for matched pairs, or or even in some cases ``), or spelling out contractions (s/doesn't/does not/), but this is fine for now.

@daniel-beck daniel-beck mentioned this pull request Jan 14, 2018
4 tasks
@oleg-nenashev
Copy link
Member

Thanks @jsoref !

@oleg-nenashev oleg-nenashev merged commit 408a66c into jenkinsci:master Jan 20, 2018
@jsoref jsoref deleted the apostro branch January 21, 2018 17:34
@recena
Copy link
Contributor

recena commented Feb 19, 2018

@jsoref I found a regression JENKINS-49634

/cc @daniel-beck @jglick @oleg-nenashev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-more-reviews Complex change, which would benefit from more eyes
Projects
None yet
5 participants