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

feat(date)!: separate timeZone method #2947

Merged
merged 14 commits into from
Jun 21, 2024
Merged

feat(date)!: separate timeZone method #2947

merged 14 commits into from
Jun 21, 2024

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Jun 12, 2024

Adds #735


Adds faker.date.timeZone() for a random global time zone.
Narrows faker.location.timeZone() to a random locale specific time zone.

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug breaking change Cannot be merged when next version is not a major release m: location Something is referring to the location module m: date Something is referring to the date module labels Jun 12, 2024
@ST-DDT ST-DDT added this to the v9.0 milestone Jun 12, 2024
@ST-DDT ST-DDT self-assigned this Jun 12, 2024
Copy link

netlify bot commented Jun 12, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit ac40c29
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/66754ac5d903000008224869
😎 Deploy Preview https://deploy-preview-2947.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.95%. Comparing base (5801cc5) to head (ac40c29).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2947      +/-   ##
==========================================
- Coverage   99.96%   99.95%   -0.01%     
==========================================
  Files        2987     2989       +2     
  Lines      216052   216099      +47     
  Branches      954      598     -356     
==========================================
+ Hits       215973   216004      +31     
- Misses         79       95      +16     
Files Coverage Δ
src/locales/base/date/index.ts 100.00% <100.00%> (ø)
src/locales/base/date/time_zone.ts 100.00% <100.00%> (ø)
src/locales/base/index.ts 100.00% <100.00%> (ø)
src/locales/base/location/time_zone.ts 100.00% <100.00%> (ø)
src/modules/date/index.ts 100.00% <100.00%> (ø)
src/modules/location/index.ts 100.00% <100.00%> (+0.32%) ⬆️

... and 1 file with indirect coverage changes

@ST-DDT ST-DDT changed the title feat(date): add/improve timeZone method feat(date)!: add/improve timeZone method Jun 12, 2024
@ST-DDT ST-DDT marked this pull request as ready for review June 12, 2024 14:51
@ST-DDT ST-DDT requested a review from a team as a code owner June 12, 2024 14:52
@ST-DDT ST-DDT requested a review from a team June 12, 2024 14:52
@ST-DDT ST-DDT linked an issue Jun 12, 2024 that may be closed by this pull request
src/definitions/date.ts Outdated Show resolved Hide resolved
src/definitions/location.ts Outdated Show resolved Hide resolved
src/modules/date/index.ts Outdated Show resolved Hide resolved
src/modules/location/index.ts Show resolved Hide resolved
@ST-DDT ST-DDT changed the title feat(date)!: add/improve timeZone method feat(date)!: separate timeZone method Jun 12, 2024
@matthewmayer
Copy link
Contributor

Why do you consider this breaking

@ST-DDT
Copy link
Member Author

ST-DDT commented Jun 12, 2024

I'm not sure I do. It might not be ! Breaking but noteworthy for the migration guide nontheless.

I think it could be considered breaking because it narrows the spec for location.timezone.

Shinigami92
Shinigami92 previously approved these changes Jun 13, 2024
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@matthewmayer matthewmayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a few time zones for a few "easy" locales just to check it works as expected?

src/modules/location/index.ts Show resolved Hide resolved
@ST-DDT
Copy link
Member Author

ST-DDT commented Jun 13, 2024

Should we add a few time zones for a few "easy" locales just to check it works as expected?

IMO we can do that in one or more separate PRs.

Shinigami92
Shinigami92 previously approved these changes Jun 13, 2024
@ST-DDT ST-DDT requested review from matthewmayer and removed request for matthewmayer June 14, 2024 14:17
@matthewmayer
Copy link
Contributor

Could we add a test to check that all locale timezone definitions are subsets of the base list?

@ST-DDT ST-DDT dismissed stale reviews from xDivisionByZerox and Shinigami92 via 082a882 June 16, 2024 14:14
@ST-DDT ST-DDT requested review from Shinigami92, xDivisionByZerox and a team June 16, 2024 14:15
@ST-DDT
Copy link
Member Author

ST-DDT commented Jun 16, 2024

Could we add a test to check that all locale timezone definitions are subsets of the base list?

Done

test/modules/location.spec.ts Outdated Show resolved Hide resolved
test/modules/location.spec.ts Show resolved Hide resolved
@ST-DDT ST-DDT enabled auto-merge (squash) June 21, 2024 09:41
@ST-DDT ST-DDT merged commit d6924f7 into next Jun 21, 2024
23 checks passed
@ST-DDT ST-DDT deleted the feat/timeZone branch June 21, 2024 09:44
eLoyyyyy pushed a commit to eLoyyyyy/faker that referenced this pull request Oct 14, 2024
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 m: date Something is referring to the date module m: location Something is referring to the location module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose or move timezone method from/to date module
4 participants