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

(GroovyTemplates) Use English Locale in the date formatter #256

Merged
merged 1 commit into from Jul 2, 2016

Conversation

Projects
None yet
4 participants
@jmini

jmini commented Jan 7, 2016

The function Date#format(java.lang.String) used in the GroovyTemplates uses the system Locale to format the date. When the Date format uses MMMM or MMM the name (resp. the short name) of the month is used.

This make the JBake build platform dependent (for example: the locale configuration on my machine is German and it is English on my CI Server)

In my opinion, for those cases, the complete form of the formatter should be used:

new java.text.SimpleDateFormat(<Pattern>, Locale.ENGLISH).format(<Date>)

This pull request makes the corresponding changes. If you prefer a different solution, I would be happy to implement it. Please provide some pointers in order for me to start.

@mikolak-net

This comment has been minimized.

Show comment
Hide comment
@mikolak-net

mikolak-net Jan 8, 2016

+1 for this. Just had to work around a problem where an RSS aggregator was malfunctioning due to non-English-localized dates present on my feed.

Of course, a better solution would be to have the date formatting local configurable, but this PR is a good first step.

mikolak-net commented Jan 8, 2016

+1 for this. Just had to work around a problem where an RSS aggregator was malfunctioning due to non-English-localized dates present on my feed.

Of course, a better solution would be to have the date formatting local configurable, but this PR is a good first step.

@jonbullock

This comment has been minimized.

Show comment
Hide comment
@jonbullock

jonbullock Jan 13, 2016

Member

Thanks @jmini - just to double check your goal of this PR was to fix the tests? Not the example Groovy templates for creating a new project/site?

I'd be happy if somebody wants to make the Locale used for Date formatting configurable via jbake.properties like the Date pattern is currently.

Member

jonbullock commented Jan 13, 2016

Thanks @jmini - just to double check your goal of this PR was to fix the tests? Not the example Groovy templates for creating a new project/site?

I'd be happy if somebody wants to make the Locale used for Date formatting configurable via jbake.properties like the Date pattern is currently.

@jmini

This comment has been minimized.

Show comment
Hide comment
@jmini

jmini Jan 13, 2016

I wanted to update the Groovy templates used when a new project is created.
=> from your comment I understand that I am not at the correct location.

jbake.properties also seems a good Idea, but I think that this is for the templates that are displayed to the user (archive, index, post, tags). The feed template always need to use english (see @mikolak-net comment).

jmini commented Jan 13, 2016

I wanted to update the Groovy templates used when a new project is created.
=> from your comment I understand that I am not at the correct location.

jbake.properties also seems a good Idea, but I think that this is for the templates that are displayed to the user (archive, index, post, tags). The feed template always need to use english (see @mikolak-net comment).

@jonbullock

This comment has been minimized.

Show comment
Hide comment
@jonbullock

jonbullock Jan 14, 2016

Member

Ahh in that case this is the repo you want: https://github.com/jbake-org/jbake-example-project-groovy

Member

jonbullock commented Jan 14, 2016

Ahh in that case this is the repo you want: https://github.com/jbake-org/jbake-example-project-groovy

@jmini

This comment has been minimized.

Show comment
Hide comment
@jmini

jmini Jan 14, 2016

I have opened a new pull request in the other repository: jbake-example-project-groovy/#2

jmini commented Jan 14, 2016

I have opened a new pull request in the other repository: jbake-example-project-groovy/#2

@jonbullock jonbullock added the task label Jan 15, 2016

@jonbullock jonbullock added this to the v2.5.0 milestone Jan 15, 2016

@jonbullock

This comment has been minimized.

Show comment
Hide comment
@jonbullock

jonbullock Jan 15, 2016

Member

Thanks I'll still take this change in to ensure the tests act the same way regardless of the machine running them.

Member

jonbullock commented Jan 15, 2016

Thanks I'll still take this change in to ensure the tests act the same way regardless of the machine running them.

@jonbullock jonbullock self-assigned this Jan 15, 2016

@ancho

This comment has been minimized.

Show comment
Hide comment
@ancho

ancho Jan 16, 2016

Member

Hmm. Maybe we should configure the locale in default.properties like

locale=en_GB

We could then set the default locale in Oven or in Main after loading the configuration with
Locale.setDefault(new Locale( config.getString("locale"));. What do you think?

Member

ancho commented Jan 16, 2016

Hmm. Maybe we should configure the locale in default.properties like

locale=en_GB

We could then set the default locale in Oven or in Main after loading the configuration with
Locale.setDefault(new Locale( config.getString("locale"));. What do you think?

@mikolak-net

This comment has been minimized.

Show comment
Hide comment
@mikolak-net

mikolak-net Jan 16, 2016

@ancho : there should be a separate setting for the global locale, and another one for machine-readable stuff such as RSS.

mikolak-net commented Jan 16, 2016

@ancho : there should be a separate setting for the global locale, and another one for machine-readable stuff such as RSS.

@ancho

This comment has been minimized.

Show comment
Hide comment
@ancho

ancho Jan 16, 2016

Member

@mikolak-net like rendering all the pages with french locale settings and rss with english locale settings?

maybe optional locale settings per file? like feed.file=feed.xml and feed.locale=en_US?

Member

ancho commented Jan 16, 2016

@mikolak-net like rendering all the pages with french locale settings and rss with english locale settings?

maybe optional locale settings per file? like feed.file=feed.xml and feed.locale=en_US?

@mikolak-net

This comment has been minimized.

Show comment
Hide comment
@mikolak-net

mikolak-net Jan 16, 2016

@ancho well, regarding the feed, after a quick read of the RSS Spec, I no longer think that a user-configurable option is the way to go in this case. It appears that valid RSS always requires non-localized dates in an RFC822-compatible format.

mikolak-net commented Jan 16, 2016

@ancho well, regarding the feed, after a quick read of the RSS Spec, I no longer think that a user-configurable option is the way to go in this case. It appears that valid RSS always requires non-localized dates in an RFC822-compatible format.

@ancho

This comment has been minimized.

Show comment
Hide comment
@ancho

ancho Jan 16, 2016

Member

Well, thank you.

But that is not the only syndication format out there. The atom syndication format requires rfc3339 time and date formats as described in rfc4278 date constucts

I think it's best to make it configurable by type. Not just the locale but the dateformat as well.

Member

ancho commented Jan 16, 2016

Well, thank you.

But that is not the only syndication format out there. The atom syndication format requires rfc3339 time and date formats as described in rfc4278 date constucts

I think it's best to make it configurable by type. Not just the locale but the dateformat as well.

@mikolak-net

This comment has been minimized.

Show comment
Hide comment
@mikolak-net

mikolak-net Jan 16, 2016

@ancho you're right. And sorry, I completely neglected to check out the Atom spec. Yeah, the solution you proposed doesn't sound like YAGNI anymore, so 👍 from me.

mikolak-net commented Jan 16, 2016

@ancho you're right. And sorry, I completely neglected to check out the Atom spec. Yeah, the solution you proposed doesn't sound like YAGNI anymore, so 👍 from me.

@ancho

This comment has been minimized.

Show comment
Hide comment
@ancho

ancho Jan 16, 2016

Member

@mikolak-net Nothing to feel sorry about. Thanks for the discussion and your effort. You pointed me and my thoughts in the right direction. I call that teamwork :)
I'll open an issue with a feature request derived from our discussion (maybe tomorrow).

Member

ancho commented Jan 16, 2016

@mikolak-net Nothing to feel sorry about. Thanks for the discussion and your effort. You pointed me and my thoughts in the right direction. I call that teamwork :)
I'll open an issue with a feature request derived from our discussion (maybe tomorrow).

@mikolak-net

This comment has been minimized.

Show comment
Hide comment
@mikolak-net

mikolak-net Jan 16, 2016

Glad to hear that :).

mikolak-net commented Jan 16, 2016

Glad to hear that :).

@jonbullock jonbullock merged commit 81c37e2 into jbake-org:master Jul 2, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

jonbullock added a commit that referenced this pull request Jul 2, 2016

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