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

refactor(locale)!: remove unused separator entry #1731

Merged
merged 6 commits into from
Jan 29, 2023

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Jan 11, 2023

The separator is currently unused and doesn't really contain a separator, instead it contains a combinator: ' & '.

This PR removes the separator from the definitions, the locale data and related scripts.


A following PR will move the title to a new metadata category to remove any exceptions from the locale: { [category: string]: { [entry: string]: any }} data structures. (To be able to remove the special case handling currently required for those fields).

@ST-DDT ST-DDT added c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent c: locale Permutes locale definitions labels Jan 11, 2023
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Jan 11, 2023
@ST-DDT ST-DDT requested review from a team January 11, 2023 20:50
@ST-DDT ST-DDT self-assigned this Jan 11, 2023
@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Merging #1731 (90a6db4) into next (6772b00) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #1731   +/-   ##
=======================================
  Coverage   99.64%   99.65%           
=======================================
  Files        2340     2340           
  Lines      242669   242653   -16     
  Branches     1111     1111           
=======================================
- Hits       241819   241804   -15     
+ Misses        829      828    -1     
  Partials       21       21           
Impacted Files Coverage Δ
src/definitions/definitions.ts 0.00% <ø> (ø)
src/locales/ar/index.ts 100.00% <ø> (ø)
src/locales/az/index.ts 100.00% <ø> (ø)
src/locales/el/index.ts 100.00% <ø> (ø)
src/locales/en/index.ts 100.00% <ø> (ø)
src/locales/es_MX/index.ts 100.00% <ø> (ø)
src/locales/ge/index.ts 100.00% <ø> (ø)
src/locales/he/index.ts 100.00% <ø> (ø)
src/locales/hy/index.ts 100.00% <ø> (ø)
src/locales/lv/index.ts 100.00% <ø> (ø)
... and 4 more

@ST-DDT ST-DDT changed the title chore: remove unused separator refactor: remove unused separator Jan 11, 2023
@ST-DDT ST-DDT added c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs and removed c: chore PR that doesn't affect the runtime behavior labels Jan 11, 2023
Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

This is a breaking change!

@xDivisionByZerox xDivisionByZerox added the breaking change Cannot be merged when next version is not a major release label Jan 11, 2023
@ST-DDT ST-DDT changed the title refactor: remove unused separator refactor(locale)!: remove unused separator Jan 11, 2023
@ST-DDT ST-DDT changed the title refactor(locale)!: remove unused separator refactor(locale)!: remove unused separator entry Jan 11, 2023
@ST-DDT ST-DDT requested a review from a team January 11, 2023 21:32
@Shinigami92
Copy link
Member

I'm not sure if this was used for helpers.fake to generate text in different locales.

But nowadays there is something like this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat
which has a conjunction and disjunction parameter
so maybe this is more likely to be used but has no support for helpers.fake anymore

@ST-DDT
Copy link
Member Author

ST-DDT commented Jan 14, 2023

I'm not sure if this was used for helpers.fake to generate text in different locales.

It isn't supported there either.

faker.helpers.fake("{{separator}}")
[Error]: Invalid module method or definition: separator
- faker.separator is not a function
- faker.definitions.separator is not an array

We don't use it and the only way to actually access it is via faker.definitions.separator directly.

That's why I would like to remove it.

@Shinigami92 Shinigami92 added the s: needs decision Needs team/maintainer decision label Jan 21, 2023
@Shinigami92
Copy link
Member

I would like to discuss this in a meeting and want to raise the question if we need an alternative
Maybe we broke something from pre-v6 times and just missed something

@xDivisionByZerox xDivisionByZerox removed the s: needs decision Needs team/maintainer decision label Jan 26, 2023
@xDivisionByZerox
Copy link
Member

In today meeting we ensured that this entry was never used and can be removed savely.

I would still suggest to mark this as breaking change.

@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Jan 26, 2023
@ST-DDT ST-DDT merged commit 0bc6c2f into next Jan 29, 2023
@ST-DDT ST-DDT deleted the chore/remove-unused-separator-locale branch January 29, 2023 15:27
matthewmayer pushed a commit to matthewmayer/faker that referenced this pull request Feb 18, 2023
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: locale Permutes locale definitions c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants