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(name.findName): rename to fullName #1127

Merged
merged 3 commits into from
Jul 19, 2022

Conversation

xDivisionByZerox
Copy link
Member

This PR renames the existing faker.name.findName() to faker.name.fullName(), while keeping the existing function but marking them as deprecated.
I also changed the signature of fullName() to an object for maintainability purposes.

I had a discussion with @Shinigami92 last week about deprecations. We came to the conclusion that they should be categorized as refactor instead of chore, what I will start to implement with this PR.

@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs labels Jul 4, 2022
@xDivisionByZerox xDivisionByZerox added this to the v7 - Current Major milestone Jul 4, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team July 4, 2022 15:00
@xDivisionByZerox xDivisionByZerox self-assigned this Jul 4, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner July 4, 2022 15:00
@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #1127 (00f0d17) into main (ea91fe6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 00f0d17 differs from pull request most recent head 663d986. Consider uploading reports for the commit 663d986 to get more accurate results

@@           Coverage Diff           @@
##             main    #1127   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files        2146     2146           
  Lines      230401   230432   +31     
  Branches      977      976    -1     
=======================================
+ Hits       229585   229619   +34     
+ Misses        795      792    -3     
  Partials       21       21           
Impacted Files Coverage Δ
src/modules/name/index.ts 100.00% <100.00%> (+1.08%) ⬆️
src/locales/en/name/gender.ts 100.00% <0.00%> (ø)

ST-DDT
ST-DDT previously approved these changes Jul 5, 2022
import-brain
import-brain previously approved these changes Jul 6, 2022
@xDivisionByZerox xDivisionByZerox force-pushed the refactor/name/rename-find-name-to-full-name branch from 9924bdf to 0af76ac Compare July 6, 2022 16:50
@xDivisionByZerox
Copy link
Member Author

This is a none blocking/breaking change. So, can we merge this?

pkuczynski
pkuczynski previously approved these changes Jul 7, 2022
@ST-DDT ST-DDT dismissed stale reviews from import-brain and themself via 0af76ac July 8, 2022 07:42
ST-DDT
ST-DDT previously approved these changes Jul 9, 2022
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Jul 9, 2022
@hankucz
Copy link
Contributor

hankucz commented Jul 18, 2022

Would this be merged anytime soon, as I am working on this module and I am afraid of conflicts?

@xDivisionByZerox
Copy link
Member Author

TBH, I don't see what's blocking the merge. Most likely just leaving this open in case anyone still wants a change. Often, many changes are merged in the release week. So I guess in one or two weeks.

@pkuczynski
Copy link
Member

Lets just merge it then! No point to keep it hanging. @ST-DDT @Shinigami92

@xDivisionByZerox xDivisionByZerox dismissed stale reviews from ST-DDT and pkuczynski via ce00b3f July 18, 2022 21:34
@xDivisionByZerox xDivisionByZerox force-pushed the refactor/name/rename-find-name-to-full-name branch from 00f0d17 to ce00b3f Compare July 18, 2022 21:34
@Shinigami92 Shinigami92 enabled auto-merge (squash) July 19, 2022 08:02
@Shinigami92 Shinigami92 merged commit 316f61f into main Jul 19, 2022
@xDivisionByZerox xDivisionByZerox deleted the refactor/name/rename-find-name-to-full-name branch July 19, 2022 08:51
hankucz pushed a commit to hankucz/faker that referenced this pull request Jul 19, 2022
@xDivisionByZerox xDivisionByZerox added the m: person Something is referring to the person module label Jul 28, 2022
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 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.

6 participants