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.

Sign up for GitHub

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

Jump to bottom

feat(company): move name formats to locales #1293

Merged
merged 7 commits into from Aug 22, 2022
Merged

feat(company): move name formats to locales #1293

merged 7 commits into from Aug 22, 2022

Conversation

Minozzzi
Copy link
Contributor

The oldest PR: #1279
Fixes : #1179

@Minozzzi Minozzzi requested a review from a team as a code owner August 22, 2022 02:22
@codecov
Copy link

codecov bot commented Aug 22, 2022 •
edited

Codecov Report

Merging #1293 (e8d769b) into main (0866ee9) will decrease coverage by 0.00%.
The diff coverage is 92.64%.

❗ Current head e8d769b differs from pull request most recent head ebcd6c1. Consider uploading reports for the commit ebcd6c1 to get more accurate results

@@            Coverage Diff             @@
##             main    #1293      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files        2160     2160              
  Lines      240431   240439       +8     
  Branches     1007     1007              
==========================================
- Hits       239539   239535       -4     
- Misses        871      883      +12     
  Partials       21       21
Impacted Files Coverage Δ
src/definitions/company.ts 0.00% <0.00%> (ø)
src/locales/az/company/name_patterns.ts 100.00% <ø> (ø)
src/locales/cz/company/name_patterns.ts 100.00% <ø> (ø)
src/locales/de/company/name_patterns.ts 100.00% <ø> (ø)
src/locales/de_AT/company/name_patterns.ts 100.00% <ø> (ø)
src/locales/de_CH/company/name_patterns.ts 100.00% <ø> (ø)
src/locales/el/company/name_patterns.ts 100.00% <ø> (ø)
src/locales/en_GH/company/name_patterns.ts 100.00% <ø> (ø)
src/locales/es/company/name_patterns.ts 100.00% <ø> (ø)
src/locales/es_MX/company/name_patterns.ts 100.00% <ø> (ø)
... and 45 more

@import-brain import-brain added 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 m: company Something is referring to the company module labels Aug 22, 2022
@import-brain import-brain requested a review from a team August 22, 2022 06:41
Shinigami92
Shinigami92 requested changes Aug 22, 2022
View reviewed changes
src/modules/company/index.ts Outdated Show resolved Hide resolved
src/modules/company/index.ts Show resolved Hide resolved
@Minozzzi @Shinigami92
refactor(modules/company/name): use ?? operator instead ternary
77a933f 
Co-authored-by: Shinigami <chrissi92@hotmail.de>
Shinigami92
Shinigami92 approved these changes Aug 22, 2022
View reviewed changes
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.

changing the todo is not so important for me, I will approve now
other maintainers can decide if we want already merge or not

@Shinigami92 Shinigami92 requested a review from a team August 22, 2022 12:12
@Shinigami92 Shinigami92 added this to the v7 - Current Major milestone Aug 22, 2022
@Shinigami92
Copy link
Member

do we need to mark this as feat:? so it shows up in changelog?

@ST-DDT ST-DDT changed the title refactor(locale): move company.name formats to locales #1279 refactor(company): move name formats to locales Aug 22, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Aug 22, 2022

do we need to mark this as feat:? so it shows up in changelog?

We don't report locale changes either. We could rename this one though.

@ST-DDT
Copy link
Member

ST-DDT commented Aug 22, 2022

@Shinigami92 Do you still consider this breaking?

@Shinigami92
Copy link
Member

We don't report locale changes either. We could rename this one though.

I thought we swapped explicitly to feat(locale): exactly for this reason?

@Shinigami92 Do you still consider this breaking?

Not anymore as there is now the fallback, the fallback can then safely removed in v8

@Minozzzi
Copy link
Contributor Author

Not anymore as there is now the fallback, the fallback can then safely removed in v8

@Shinigami92 If we create an issue for this by adding patterns for all locales. Could it be removed before v8?

@Shinigami92
Copy link
Member

Not anymore as there is now the fallback, the fallback can then safely removed in v8

@Shinigami92 If we create an issue for this by adding patterns for all locales. Could it be removed before v8?

yes

@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug and removed breaking change Cannot be merged when next version is not a major release labels Aug 22, 2022
@Shinigami92 Shinigami92 changed the title refactor(company): move name formats to locales feat(company): move name formats to locales Aug 22, 2022
@Shinigami92 Shinigami92 enabled auto-merge (squash) August 22, 2022 18:48
@Shinigami92 Shinigami92 merged commit e1f26a6 into faker-js:main Aug 22, 2022
@xDivisionByZerox xDivisionByZerox mentioned this pull request Aug 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkuczynski pkuczynski pkuczynski left review comments

@ST-DDT ST-DDT ST-DDT approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

Assignees

@Minozzzi Minozzzi

Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: company Something is referring to the company module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v7 - Current Major
Development

Successfully merging this pull request may close these issues.

Localize format patterns for company.name
5 participants
@Minozzzi @Shinigami92 @ST-DDT @pkuczynski @import-brain