Skip to content

feat(date)!: separate timeZone method#2947

Merged
ST-DDT merged 14 commits into
nextfrom
feat/timeZone
Jun 21, 2024
Merged

feat(date)!: separate timeZone method#2947
ST-DDT merged 14 commits into
nextfrom
feat/timeZone

Conversation

@ST-DDT

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

Copy link
Copy Markdown
Member

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
@netlify

netlify Bot commented Jun 12, 2024

Copy link
Copy Markdown

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.

@codecov

codecov Bot commented Jun 12, 2024

Copy link
Copy Markdown

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
Comment thread src/definitions/date.ts Outdated
Comment thread src/definitions/location.ts Outdated
Comment thread src/modules/date/index.ts Outdated
Comment thread src/modules/location/index.ts
@ST-DDT ST-DDT changed the title feat(date)!: add/improve timeZone method feat(date)!: separate timeZone method Jun 12, 2024
@matthewmayer

Copy link
Copy Markdown
Contributor

Why do you consider this breaking

@ST-DDT

ST-DDT commented Jun 12, 2024

Copy link
Copy Markdown
Member Author

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

@Shinigami92 Shinigami92 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@matthewmayer matthewmayer left a comment

Copy link
Copy Markdown
Contributor

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?

Comment thread src/modules/location/index.ts
@ST-DDT

ST-DDT commented Jun 13, 2024

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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 a team, Shinigami92 and xDivisionByZerox June 16, 2024 14:15
@ST-DDT

ST-DDT commented Jun 16, 2024

Copy link
Copy Markdown
Member Author

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

Done

Comment thread test/modules/location.spec.ts Outdated
Comment thread test/modules/location.spec.ts
@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
@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

4 participants