Skip to content

refactor(types): rename locale definition types to singular#2058

Merged
ST-DDT merged 3 commits into
nextfrom
refactor/rename-locale-definition-types
May 1, 2023
Merged

refactor(types): rename locale definition types to singular#2058
ST-DDT merged 3 commits into
nextfrom
refactor/rename-locale-definition-types

Conversation

@ST-DDT

@ST-DDT ST-DDT commented Apr 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Rename:

  • LocaleDefinition -> LocaleDefinitions
  • LocaleEntry -> LocaleCategoryDefinitions (cause it is only for categories)

I'm not sure about the new names and whether we should use singular or plural names for these.
@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs breaking change Cannot be merged when next version is not a major release labels Apr 16, 2023
@ST-DDT ST-DDT requested review from a team April 16, 2023 12:27
@ST-DDT ST-DDT self-assigned this Apr 16, 2023
@ST-DDT ST-DDT requested a review from a team April 16, 2023 12:27
@codecov

codecov Bot commented Apr 16, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2058 (9f7407c) into next (b72d52e) will decrease coverage by 0.01%.
The diff coverage is 87.11%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #2058      +/-   ##
==========================================
- Coverage   99.59%   99.59%   -0.01%     
==========================================
  Files        2567     2567              
  Lines      243357   243388      +31     
  Branches     1254     1251       -3     
==========================================
+ Hits       242383   242412      +29     
- Misses        947      949       +2     
  Partials       27       27
Impacted Files Coverage Δ
src/definitions/airline.ts 0.00% <0.00%> (ø)
src/definitions/animal.ts 0.00% <0.00%> (ø)
src/definitions/color.ts 0.00% <0.00%> (ø)
src/definitions/commerce.ts 0.00% <0.00%> (ø)
src/definitions/company.ts 0.00% <0.00%> (ø)
src/definitions/database.ts 0.00% <0.00%> (ø)
src/definitions/date.ts 0.00% <0.00%> (ø)
src/definitions/definitions.ts 0.00% <0.00%> (ø)
src/definitions/finance.ts 0.00% <0.00%> (ø)
src/definitions/hacker.ts 0.00% <0.00%> (ø)
... and 289 more

... and 3 files with indirect coverage changes
@Shinigami92

Shinigami92 commented Apr 20, 2023

Copy link
Copy Markdown
Member

Especially locale: LocaleDefinitions | LocaleDefinitions[] is a good indicator to me that we should use singular (I'm sorry 🤷)
@Shinigami92 Shinigami92 added the s: needs decision Needs team/maintainer decision label Apr 20, 2023
@ST-DDT

ST-DDT commented Apr 27, 2023

Copy link
Copy Markdown
Member Author

We should decide this before releasing v8.0
@Shinigami92

Shinigami92 commented Apr 27, 2023

Copy link
Copy Markdown
Member

We should decide this before releasing v8.0

I vote for singular
@Shinigami92

Shinigami92 commented Apr 30, 2023

Copy link
Copy Markdown
Member

@xDivisionByZerox What do you think is the better option?
@xDivisionByZerox

xDivisionByZerox commented Apr 30, 2023

Copy link
Copy Markdown
Member

I prefer the singular variant (LocaleDefinition, LocaleCategoryDefinition).

You could say that the object, represented by the type, is a collection of keys - similar to a Map.
While you would most likely write the value of a Map in the plural form (const books = new Map();), I don't see the argument being fitting for our specific case. A locale definition may be a collection of suggested definitions but it still applies to one locale only (so singular).

Thats at least how I see it.
@ST-DDT ST-DDT force-pushed the refactor/rename-locale-definition-types branch from 184c3b8 to 671790d Compare April 30, 2023 13:17
@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Apr 30, 2023
@ST-DDT

ST-DDT commented Apr 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Changed to singular
@ST-DDT ST-DDT requested review from a team, Shinigami92 and xDivisionByZerox April 30, 2023 13:36
@matthewmayer

matthewmayer commented Apr 30, 2023

Copy link
Copy Markdown
Contributor

If this is considered a breaking change then it should be mentioned in migration guide.
@ST-DDT ST-DDT changed the title refactor(types)!: rename locale definition types refactor(types): rename locale definition types to singular Apr 30, 2023
@ST-DDT ST-DDT removed the breaking change Cannot be merged when next version is not a major release label Apr 30, 2023
@ST-DDT

ST-DDT commented Apr 30, 2023

Copy link
Copy Markdown
Member Author

If this is considered a breaking change then it should be mentioned in migration guide.

No, it is not because the old aliases are still exported.
Shinigami92
Shinigami92 previously approved these changes Apr 30, 2023
View reviewed changes

@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
xDivisionByZerox
xDivisionByZerox reviewed Apr 30, 2023
View reviewed changes
Comment thread src/definitions/definitions.ts Outdated
@ST-DDT ST-DDT requested review from a team, Shinigami92 and xDivisionByZerox April 30, 2023 16:03
@ST-DDT ST-DDT merged commit e9691fb into next May 1, 2023
@ST-DDT ST-DDT deleted the refactor/rename-locale-definition-types branch May 1, 2023 06:13
matthewmayer added a commit to matthewmayer/faker that referenced this pull request May 1, 2023
@matthewmayer
fix conflicts caused by aftermath of faker-js#2058
c5c56a4
matthewmayer added a commit to matthewmayer/faker that referenced this pull request May 1, 2023
@matthewmayer
fix conflicts caused by aftermath of faker-js#2058
f5eab34
matthewmayer added a commit to matthewmayer/faker that referenced this pull request May 1, 2023
@matthewmayer
fix conflicts caused by aftermath of faker-js#2058
8e64482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent

5 participants

@ST-DDT @Shinigami92 @xDivisionByZerox @matthewmayer @ejcheng