Skip to content

Make default locale configurable on JVM level#623

Draft
michael-o wants to merge 1 commit intomasterfrom
configure-default-locale
Draft

Make default locale configurable on JVM level#623
michael-o wants to merge 1 commit intomasterfrom
configure-default-locale

Conversation

@michael-o
Copy link
Copy Markdown
Member

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Your pull request should address just one issue, without pulling in other changes.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body.
    Note that commits might be squashed by a maintainer on merge.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
    This may not always be possible but is a best-practice.
  • Run mvn verify to make sure basic checks pass.
    A more thorough check will be performed on your pull request automatically.
  • You have run the integration tests successfully (mvn -Prun-its verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

* @see Locale#ROOT
*/
Locale DEFAULT_LOCALE = Locale.ROOT;
Locale DEFAULT_LOCALE = new Locale(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need three different properties, let us use https://docs.oracle.com/javase/8/docs/api/java/util/Locale.html#forLanguageTag-java.lang.String- and get the language tag from a single property.
Also regarding the property name I find user in it confusing. I would propose just maven.doxia.sitetools.default.locale.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Two reasons:

  • I have designed identically to Java's system properties for that
  • I didn't use the mentioned method because I didn't want to be inconsistent how to accept the locale string, namely reverse #toString().

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can merge into one properly and pull up the parse method into the interface as a static method.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two reasons:

  • I have designed identically to Java's system properties for that
  • I didn't use the mentioned method because I didn't want to be inconsistent how to accept the locale string, namely reverse #toString().

I don't particularly like the choice of JVM: https://www.oracle.com/technical-resources/articles/javase/locale.html#using. Since Locale.forLanguage(...) was introduced only with 1.7 is probably the reason why they split it up. Also the user infix is really weird, as this is not at all bound to a specific user.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

user is again from the JVM. I sticked to well-known property names.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I know, but to me the user infix is still not very reasonable. I would rather deviate from the JVM naming here

Yep, had the same feeling. It was worth a try. Will worth on a better draft end of week.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@michael-o Would like to target a release end of next week, should this be part of it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We first need to release Doxia. Do you want to do the release?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Vote already ongoing: https://lists.apache.org/thread/nhfr3nnj767w4c86gzbsm4wvc4d93j76, was not aware that this has Doxia implications.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It does not. Will try to evaluate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants