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

Use HUGO 0.87's date formatting feature with time.Format #555

Merged

Conversation

Saxodwarf
Copy link
Contributor

@Saxodwarf Saxodwarf commented Sep 5, 2021

What does this PR change? What problem does it solve?

In the post_meta.html partial file, use .Date | time.Format instead of .Date.Format to format the date, to take advantage of HUGO's new localized date formatting feature when formatting the date in the meta informations.
This time.Format's behaviour was included in HUGO 0.87, as described in the documentation.

Was the change discussed in an issue or in the Discussions before?

No

PR Checklist

  • This change adds/updates translations and I have used the template present here.
  • I have enabled maintainer edits for this PR.
  • I have verified that the code works as described/as intended.
  • This change adds a Social Icon which has a permissive license to use it.
  • This change does not include any CDN resources/links.
  • This change does not include any unrelated scripts such as bash and python scripts.
  • This change updates the overridden internal templates from HUGO's repository.

@sonarcloud
Copy link

sonarcloud bot commented Sep 5, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@RoneoOrg
Copy link

RoneoOrg commented Oct 2, 2021

Hi @Saxodwarf ,

the same Date.Format you are changing here also appears in rss.xml and post_meta.html

Do you think these occurrences should be changed too?

@Saxodwarf
Copy link
Contributor Author

Hi @RoneoOrg,

I believe the post_meta.html file I'm editing in this pull request is the one you mentioned in your comment.

I saw the use of Date.Format in rss.xml, but there, the date format is fixed and respects RFC 822 as requested in the RSS specification, so there is no need to use the new feature for date localization.

For the sake of coherence, we could edit rss.xml to use time.Format, but imho it is not useful. What do you think ?

@RoneoOrg
Copy link

RoneoOrg commented Oct 3, 2021

Hi @Saxodwarf

I believe the post_meta.html file I'm editing in this pull request is the one you mentioned in your comment.

You're right, I scanned the whole repo in its current version!

I saw the use of Date.Format in rss.xml, but there, the date format is fixed and respects RFC 822 as requested in the RSS specification, so there is no need to use the new feature for date localization.

For the sake of coherence, we could edit rss.xml to use time.Format, but imho it is not useful. What do you think ?

As .Date.Format is far from being deprecated, let's stick with the current implementation
"If it's not broken don't fix it"

Thanks for your reference of the RFC, enlightening!

@JoCat
Copy link

JoCat commented Oct 10, 2021

As .Date.Format is far from being deprecated, let's stick with the current implementation "If it's not broken don't fix it"

This is cool, but doesn't work as expected on non-English sites.

How should it work:
image

How does it work:
image

How I solved the problem:

  1. Use time.Format (https://gohugo.io/functions/dateformat/#datetime-formatting-layouts)
    As in the pull request
  2. Use .Date.Local (https://gohugo.io/functions/format/#use-local-and-utc)
    {{- $scratch.Add "meta" (slice (dateFormat (default "January 2, 2006" .Site.Params.DateFormat) .Date.Local)) }}

@Saxodwarf
Copy link
Contributor Author

Hello @JoCat

I think the issue here might have something to do with the date formatting layout.
I left the default date format in my pull request, to avoid breaking compatibility with pre-0.87 version of Hugo, but I actually set the DateFormat parameter to ":date_long" in my config.yaml file.
This layout identifier outputs a localized date, according to the documentation.

Combined with the defaultContentLanguage set to ru, I get the correct result (or I think I do, unfortunately I don't speak the language):
image
Could it do the trick for you ? Or do you already use a different customized date layout ?

I'm far from being an expert in the arcane of internationalization, so I might be missing something, but I tested both your solution and the one I proposed, and they gave me the same result with both the language parameter set to french (my mother tongue) and to Russian. Without the :date_long parameter, the date was displayed with the default format, despite having the translated month name, with your code and with mine...
In the end, I lack the expertise to really understand the difference our two lines of code, since without using the new internationalized date format string, I can't get an internationalized date format in both cases.

I guess we would need more people to try it out themselves, maybe with different languages, but as far as I'm concerned, I'm good with either your solution or mine.

@JoCat
Copy link

JoCat commented Oct 25, 2021

Hello @Saxodwarf

Yes, it works correctly, by the way, I did so initially, when I tested ))

Initially, I used the same option that you suggested, but it seemed to me that it does not correspond to what is already there (in the screenshot below, I did not know that the previous format is also supported, and not only the new one).
image

And so i offered the second option as another example of a solution to the problem.
But, as it turned out, both work (only the second option does not support the notation from the first).

Could it do the trick for you ? Or do you already use a different customized date layout ?

I'm still using what I have now and I'm waiting for the PR to be accepted and add it to the release))

@sonarcloud
Copy link

sonarcloud bot commented Nov 26, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Saxodwarf
Copy link
Contributor Author

Hi all,
I just resolved a merge conflict with the last version of master.
I hope this can be merged soon :)

@nielsbrakel
Copy link

nielsbrakel commented Dec 16, 2021

Tested it with my multilingual site and works perfect! @adityatelange can this be merged?

@adityatelange
Copy link
Owner

adityatelange commented Dec 24, 2021

@Saxodwarf @nielsbrakel Does this have backwards compatibility?

Would this change work with Hugo 0.83 or do we need to update the min Hugo version also?

It would be nice if anyone could test this..

@nielsbrakel
Copy link

nielsbrakel commented Dec 24, 2021

@adityatelange This is a new feature since 0.87 so I suspect it is not backwards compatible.

I would suggest raising the min supported version in the next major release.

@Saxodwarf
Copy link
Contributor Author

Would this change work with Hugo 0.83 or do we need to update the min Hugo version also?

@adityatelange Yes. Although the internationalized date format feature is new in 0.87, the date.time function is not.
I just tested with Hugo 0.83, and the date is correctly displayed if we do not use the new layout strings.

If we keep the current, non-internationalized default, as I did in the PR, the change should be backward-compatible, and those of us who want to use the new feature can just put a DateFormat: ":date_long"parameter, or something similar in their configuration file.
So my take is that we do not need to increase the minimum Hugo version.

@adityatelange adityatelange merged commit c59193f into adityatelange:master Dec 26, 2021
JoCat added a commit to AuroraTeam/blog.aurora-team.ru that referenced this pull request Dec 26, 2021
@RoneoOrg RoneoOrg mentioned this pull request Sep 23, 2022
7 tasks
kylethedeveloper pushed a commit to kylethedeveloper/hugo-PaperMod that referenced this pull request Feb 21, 2023
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.

6 participants