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(person)!: rename name module #1445

Merged
merged 17 commits into from
Oct 16, 2022

Conversation

Shinigami92
Copy link
Member

closes #1343

@Shinigami92 Shinigami92 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: person Something is referring to the person module labels Oct 14, 2022
@Shinigami92 Shinigami92 self-assigned this Oct 14, 2022
@Shinigami92 Shinigami92 linked an issue Oct 14, 2022 that may be closed by this pull request
@Shinigami92
Copy link
Member Author

Blocked by:

@Shinigami92 Shinigami92 added the s: on hold Blocked by something or frozen to avoid conflicts label Oct 14, 2022
@Shinigami92
Copy link
Member Author

@ST-DDT I'm not sure if we want to support faker.helpers.fake('{{name.firstName}} or {{name.first_name}}')
It might be really hard for us to maintain that in the long run
We also would need to write extra code to support that but in a way so we don't duplicate the name definitions
So you have any ideas? Or should we just make a hard breaking change?

@Shinigami92
Copy link
Member Author

Okay, this is now in a done / ready for review state, but I want to first merge the cleanup deprecations as of that will reduce some changes.
So until then, this PR will stay in draft.

@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Merging #1445 (086c23c) into next (90b9c5c) will increase coverage by 0.01%.
The diff coverage is 98.64%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1445      +/-   ##
==========================================
+ Coverage   99.62%   99.63%   +0.01%     
==========================================
  Files        2164     2164              
  Lines      236815   236854      +39     
  Branches     1003     1003              
==========================================
+ Hits       235916   235984      +68     
+ Misses        877      849      -28     
+ Partials       22       21       -1     
Impacted Files Coverage Δ
src/definitions/definitions.ts 0.00% <0.00%> (ø)
src/definitions/index.ts 0.00% <0.00%> (ø)
src/definitions/person.ts 0.00% <0.00%> (ø)
src/locales/af_ZA/person/female_first_name.ts 100.00% <ø> (ø)
src/locales/af_ZA/person/first_name.ts 100.00% <ø> (ø)
src/locales/af_ZA/person/last_name.ts 100.00% <ø> (ø)
src/locales/af_ZA/person/male_first_name.ts 100.00% <ø> (ø)
src/locales/ar/person/female_first_name.ts 100.00% <ø> (ø)
src/locales/ar/person/first_name.ts 100.00% <ø> (ø)
src/locales/ar/person/last_name.ts 100.00% <ø> (ø)
... and 649 more

src/faker.ts Outdated Show resolved Hide resolved
src/locales/de/person/name.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Oct 14, 2022
@Shinigami92 Shinigami92 removed the s: on hold Blocked by something or frozen to avoid conflicts label Oct 14, 2022
@Shinigami92 Shinigami92 force-pushed the 1343-rename-name-module-to-person branch from dd71093 to 09f53e7 Compare October 14, 2022 20:09
@Shinigami92 Shinigami92 marked this pull request as ready for review October 14, 2022 20:17
@Shinigami92 Shinigami92 requested a review from a team October 14, 2022 20:17
@Shinigami92 Shinigami92 requested a review from a team as a code owner October 14, 2022 20:17
@Shinigami92 Shinigami92 requested a review from ST-DDT October 14, 2022 20:17
@Shinigami92 Shinigami92 marked this pull request as draft October 14, 2022 20:18
@ST-DDT
Copy link
Member

ST-DDT commented Oct 14, 2022

I don't know why this works, but it works.

@Shinigami92 Shinigami92 marked this pull request as ready for review October 14, 2022 23:29
test/helpers.spec.ts Outdated Show resolved Hide resolved
test/helpers.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@Shinigami92 Shinigami92 force-pushed the 1343-rename-name-module-to-person branch from b6096f8 to 0165a90 Compare October 14, 2022 23:57
@Shinigami92 Shinigami92 requested review from ST-DDT and a team October 15, 2022 00:09
ST-DDT
ST-DDT previously approved these changes Oct 15, 2022
@matthewmayer
Copy link
Contributor

in addition to the new faker.person tests, why not keep the existing tests for the faker.name module ?

test/__snapshots__/name.spec.ts.snap
test/name.spec.ts

That will help ensure no unexpected regressions/breaking changes.

@Shinigami92
Copy link
Member Author

in addition to the new faker.person tests, why not keep the existing tests for the faker.name module ?

test/__snapshots__/name.spec.ts.snap
test/name.spec.ts

That will help ensure no unexpected regressions/breaking changes.

Because it is just an alias, behind it there is exactly the same code, so no need to test it twice.

@xDivisionByZerox
Copy link
Member

I'm a bit lost, shouldn't this follow our normal deprecation workflow? As now a user has no time-window to migrate, but have to rework their codebase. Is this desired even if this will be in a breaking change?

@xDivisionByZerox
Copy link
Member

I'm a bit lost, shouldn't this follow our normal deprecation workflow? As now a user has no time-window to migrate, but have to rework their codebase. Is this desired even if this will be in a breaking change?

Sorry, too many files. Didn't saw the getter in the Faker class. So if this follows the typical deprecation workflow, we need to keep the tests as @matthewmayer suggested. Even is if is just an alias, how do you confirm that it is that way? The test would also catch breaking changes in the NameModule which were designed to only affect the PersonModule.

@ST-DDT
Copy link
Member

ST-DDT commented Oct 16, 2022

So if this follows the typical deprecation workflow, we need to keep the tests as @matthewmayer suggested. Even is if is just an alias, how do you confirm that it is that way? The test would also catch breaking changes in the NameModule which were designed to only affect the PersonModule.

I'm not sure whether I get you right.
If we duplicate the tests for faker.name, then this would only prevent breaking changes if we freeze the name tests until after v8.0.0.
However, the same applies to the PersonModule.
If we treat it like an existing module and only change things after a deprecation phase, then there is no need for the extra tests.

@Shinigami92 Shinigami92 requested review from import-brain and a team October 16, 2022 16:30
@ST-DDT
Copy link
Member

ST-DDT commented Oct 16, 2022

TODO: Create an issue to remove the special handling in v10.0 after this is merged (to get the final line numbers and commit references).

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug p: 1-normal Nothing urgent labels Oct 16, 2022
Copy link
Member

@import-brain import-brain left a comment

Choose a reason for hiding this comment

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

LGTM :)

@Shinigami92 Shinigami92 merged commit 20f2236 into next Oct 16, 2022
@Shinigami92 Shinigami92 deleted the 1343-rename-name-module-to-person branch October 16, 2022 20:05
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: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: person Something is referring to the person module 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.

Rename Name module to Person
5 participants