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

Expose or move timezone method from/to date module #735

Closed
ST-DDT opened this issue Mar 30, 2022 · 12 comments · Fixed by #2947
Closed

Expose or move timezone method from/to date module #735

ST-DDT opened this issue Mar 30, 2022 · 12 comments · Fixed by #2947
Assignees
Labels
breaking change Cannot be merged when next version is not a major release c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Mar 30, 2022

Clear and concise description of the problem

The timezone method is more closely related to dates then it is to timezones.

Suggested solution

Move the timezone method to the date module.

Alternative

Expose them from both modules.

Additional context

No response

@Shinigami92
Copy link
Member

Shinigami92 commented Sep 9, 2022

address will be renamed to location (#1344) 🤔
now the question: is timezone a good fit for location?

@ST-DDT
Copy link
Member Author

ST-DDT commented Sep 22, 2022

Team proposal

  • The date/time module will return any timezone.
  • The location module will return a timezone matching to the selected locale/country.

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 10, 2022

Team decision

We will implement that proposal.

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Nov 10, 2022
@Shinigami92
Copy link
Member

In #1678 we deleted all non en timezones and enhanced the en one. In a PR for this issue we then need to extract it out into a global thing and also need to think about how to "filter"? them by selected locale

@matthewmayer
Copy link
Contributor

You could just have a test to check that any locale specific rules are subsets of the global list.

@Shinigami92
Copy link
Member

You could just have a test to check that any locale specific rules are subsets of the global list.

I would make it less complex and just merge all specific to one global 🤔
That way no tests are even needed, are they?

@ST-DDT
Copy link
Member Author

ST-DDT commented Dec 24, 2022

and also need to think about how to "filter"? them by selected locale

There is no way to automatically filter them.
The per locale files have to be created by hand (probably once someone actually needs one).

I would make it less complex and just merge all specific to one global 🤔

I'm confused here. What should we do with that global one? IMO comparing the locale specific timezones to the global IANA list at build time is a good and easy to understand solution.

@Shinigami92
Copy link
Member

I was not aware that it is fully auto-generated
I just saw the file src/locales/en/location/time_zone.ts that doesn't have a comment that it was auto-generated and also saw no code in our source code to regenerate it and therefore I just thought it can be edited by the next human that comes along

@ST-DDT
Copy link
Member Author

ST-DDT commented Dec 24, 2022

that doesn't have a comment that it was auto-generated

Its written in the PR description.
I dont expect it to change in the future (once it is a global).

@Shinigami92
Copy link
Member

Its written in the PR description.

This is lost in the deep void of space to me. The PR is merged and I as a user or contributor don't want to search trough every potential associated issue/PR to understand what was going on in history or which decisions were made for that.

I dont expect it to change in the future (once it is a global).

I agree

@matthewmayer
Copy link
Contributor

tz zones do very occasionally change, perhaps 1 or 2 new zones per year.

For example a new zone America/Ciudad_Juarez was added in November.

https://data.iana.org/time-zones/tzdb/NEWS

But it's slow enough that I think updates can be done manually. And the fact that faker might be missing a few minor zones is not important.

@matthewmayer
Copy link
Contributor

Probably makes sense to add the two lines of code I used to generate as a comment at the top of the file in case someone wants to use this in the future

const {timeZonesNames} = require("@vvo/tzdb")
console.log(JSON.stringify(timeZonesNames, null, 2))



@ST-DDT ST-DDT removed this from the v8.0 - Module Re-Shuffling milestone Jan 26, 2023
@ST-DDT ST-DDT modified the milestones: v8.x, v9.0 Oct 10, 2023
@ST-DDT ST-DDT added the breaking change Cannot be merged when next version is not a major release label Oct 10, 2023
@ST-DDT ST-DDT self-assigned this Jun 6, 2024
@ST-DDT ST-DDT linked a pull request Jun 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

3 participants