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

time: Fix toml zoneinfo inconsistency #10157

Closed
wants to merge 7 commits into from

Conversation

satotake
Copy link
Contributor

@satotake satotake commented Aug 7, 2022

TOML's time representation has known issues about zoneinfo name such as PDT in hugo

These are caused because

  • TOML's time format has only offset
    • zoneinfo cannot be retrieved with offset value
  • TZ environment variable can change the behaviour
  • cast.ToTimeInDefaultLocationE may or may not use location arg

Given that hugo often is used on both local and remote machine, and as #9996 said hugo supports multilingual site generation,

  • TZ envvar should not have any effect on this
    • Remote environments can be hard to debug
    • UPDATE: If the site config does not have timezone, use time.Local (exception)
  • If only timeZone is set explicitly in config file and offsets of
    time are matched with it, hugo should show zoneinfo
    • Otherwise, zoneinfo should not be displayed
  • hugo should handle TOML's time object and time string equally

To meet these,

  • assure to set location derived from conf to time of TOML if possible
  • add tests

Close #9996

TOML's time representation has known issues about zoneinfo name such as `PDT`

- gohugoio#9082
- gohugoio#9982
- gohugoio#8895
- gohugoio#9996

These are caused because

* TOML's time format has only offset
    - zoneinfo cannot be retrieved with offset value
* `TZ` environment variable can change the behaviour
* `cast.ToTimeInDefaultLocationE` may or may not use `location` arg

Given that hugo often is used on both local and remote machine, and as

* `TZ` envvar should not have any effect on this
   - Remote environments can be hard to debug
* If only `timeZone` is set explicitly in config file and offsets of
  time are matched with it, hugo should show zoneinfo
   - Otherwise, zoneinfo should not be displayed
* hugo should handle TOML's time object and time string equally

To meet these,

* assure to set `location` derived from conf to time of TOML if possible
* add tests

Close gohugoio#9996
@satotake satotake changed the title time: Add SetLocationIfOffsetMatched time: Fix toml zoneinfo inconsistency Aug 7, 2022
Changing `TZ` between tests has no effect because the initialization
only occurs once during running.

- Change `time.Local` global variable between tests
- Fix and add test cases
- Fix implementation
@bep
Copy link
Member

bep commented Aug 22, 2022

@satotake thanks a lot for this. The code looks good to me, even if this "time landscape" is confusing me pretty hard.

@jmooring would you have time to take this PR for a spin and see if this fix makes sense to you?

@jmooring
Copy link
Member

jmooring commented Aug 23, 2022

Sorry, but I don't understand the commit message "time: Fix toml zoneinfo inconsistency". This PR seems to be more about making this configuration work by ignoring the OS tzdata:

[languages.en]
languageCode = 'en-US'
languageName = 'English'
timeZone = 'America/Los_Angeles'
weight = 1

[languages.es]
languageCode = 'es-ES'
languageName = 'Espanol'
timeZone = 'Europe/Madrid'
weight = 2

Most sites have not set timeZone in site configuration, so this is a breaking change. The time zone abbreviations that are visible in <= v0.101.0 will not appear after this change unless site authors add timeZone to their site configuration. I'm fine with that, but we need to let people know.

@bep
Copy link
Member

bep commented Aug 23, 2022

Most sites have not set timeZone in site configuration, so this is a breaking change.

We could possibly apply this fix only when the locale is established from config. Not sure.

@satotake
Copy link
Contributor Author

Most sites have not set timeZone in site configuration, so this is a breaking change

We could possibly apply this fix only when the locale is established from config. Not sure.

It was my intention to show no zone info if config does not have timeZone (considering remote/local site building).
But I changed that. If the site config has no timeZone, try to use time.Local timezone.

@github-actions
Copy link

This PR has been automatically marked as stale because it has not had recent activity. The resources of the Hugo team are limited, and so we are asking for your help.
Please check https://github.com/gohugoio/hugo/blob/master/CONTRIBUTING.md#code-contribution and verify that this code contribution fits with the description. If yes, tell is in a comment.
This PR will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

@github-actions github-actions bot added the Stale label Aug 25, 2023
@github-actions github-actions bot closed this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date format / timezone issue after remote build
3 participants