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(company): rename companyName to name #1166

Merged

Conversation

Minozzzi
Copy link
Contributor

@Minozzzi Minozzzi commented Jul 19, 2022

Hi guys, I applied te deprecated workflow on function company.companyName(). And I have a suggestion to the new function created, the parameters could be an object instead array positions. What do you guys think?

Related issue: #1142

@Minozzzi Minozzzi requested a review from a team as a code owner July 19, 2022 02:26
@Minozzzi Minozzzi changed the title Refactor/company/change company name to name refactor(company) Jul 19, 2022
@Minozzzi Minozzzi changed the title refactor(company) refactor(company): rename companyName to name Jul 19, 2022
@Shinigami92
Copy link
Member

We are already really slowly migrate nearly all functions to using an options object as parameter, so yes 🙂

Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Instead of just copying the method you should call the new method from the old one.

@xDivisionByZerox xDivisionByZerox added c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs s: accepted Accepted feature / Confirmed bug labels Jul 19, 2022
@xDivisionByZerox xDivisionByZerox added this to the v7 - Current Major milestone Jul 19, 2022
@xDivisionByZerox xDivisionByZerox added the p: 1-normal Nothing urgent label Jul 19, 2022
Copy link
Member

@ejcheng ejcheng left a comment

Choose a reason for hiding this comment

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

Hmm, deprecation tests are failing...👀

I'm not too familiar with how those tests work, does anyone have any idea why it failed?

@ST-DDT
Copy link
Member

ST-DDT commented Jul 20, 2022

FAIL test/scripts/apidoc/examplesAndDeprecations.spec.ts > examples and deprecations > Finance > transactionDescription
AssertionError: expected "warn" to not be called at allexpected "warn" to not be called at all
Received:
1st warn call:
Array [
"[@faker-js/faker]: faker.company.companyName() is deprecated. Please use faker.company.name() instead..",
]

It looks like finance.transactionDescription still calls company.companyName.

const company = this.faker.company.companyName();

@xDivisionByZerox xDivisionByZerox self-requested a review July 20, 2022 07:14
@xDivisionByZerox xDivisionByZerox dismissed their stale review July 20, 2022 07:15

Found missing deprecation properties.

@Minozzzi
Copy link
Contributor Author

FAIL test/scripts/apidoc/examplesAndDeprecations.spec.ts > examples and deprecations > Finance > transactionDescription
AssertionError: expected "warn" to not be called at allexpected "warn" to not be called at all
Received:
1st warn call:
Array [
"[@faker-js/faker]: faker.company.companyName() is deprecated. Please use faker.company.name() instead..",
]

It looks like finance.transactionDescription still calls company.companyName.

const company = this.faker.company.companyName();

We should call company.name() on this module?

@xDivisionByZerox
Copy link
Member

Yes, our functions should always call the latest faker functions.

@Minozzzi
Copy link
Contributor Author

Corrected your comments 😁

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #1166 (32b30d6) into main (fbddaa3) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1166      +/-   ##
==========================================
- Coverage   99.64%   99.64%   -0.01%     
==========================================
  Files        2146     2146              
  Lines      230467   230491      +24     
  Branches      980      980              
==========================================
+ Hits       229657   229678      +21     
- Misses        789      792       +3     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/company/index.ts 100.00% <100.00%> (ø)
src/modules/finance/index.ts 99.31% <100.00%> (-0.69%) ⬇️

ejcheng
ejcheng previously approved these changes Jul 20, 2022
@ST-DDT ST-DDT merged commit 3afc793 into faker-js:main Jul 22, 2022
@ST-DDT ST-DDT linked an issue Jul 22, 2022 that may be closed by this pull request
@xDivisionByZerox xDivisionByZerox added the m: company Something is referring to the company 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: company Something is referring to the company 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 company.companyName() to company.name()
5 participants