Skip to content

Make default locale configurable on JVM level #623

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.

Sign up for GitHub

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

Draft
michael-o wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Make default locale configurable on JVM level #623

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion doxia-integration-tools/src/main/java/org/apache/maven/doxia/tools/SiteTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ public interface SiteTool {
*
* @see Locale#ROOT
*/
Locale DEFAULT_LOCALE = Locale.ROOT;
Locale DEFAULT_LOCALE = new Locale(
Copy link
Copy Markdown
Member

@kwin kwin Mar 2, 2026

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

@michael-o michael-o Mar 2, 2026

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

@michael-o michael-o Mar 2, 2026

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

@kwin kwin Mar 2, 2026

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

@michael-o michael-o Mar 2, 2026

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

@michael-o michael-o Mar 2, 2026

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

@kwin kwin Mar 17, 2026

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

@michael-o michael-o Mar 17, 2026

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

@kwin kwin Mar 17, 2026

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

@michael-o michael-o Mar 17, 2026

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.

System.getProperty("maven.doxia.sitetools.user.language", ""),
System.getProperty("maven.doxia.sitetools.user.country", ""),
System.getProperty("maven.doxia.sitetools.user.variant", ""));

/**
* Get a skin artifact from one of the repositories.
Expand Down