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(datatype): standardize arguments #1804

Merged
merged 5 commits into from Feb 4, 2023

Conversation

xDivisionByZerox
Copy link
Member

Part of #1349.

Standardize all function signatures in the DatatypeModule to support an options object.

@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 m: datatype Something is referring to the datatype module labels Feb 1, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team February 1, 2023 18:47
@xDivisionByZerox xDivisionByZerox self-assigned this Feb 1, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner February 1, 2023 18:47
@xDivisionByZerox
Copy link
Member Author

Should this allow for a number argument?

hexadecimal(

@xDivisionByZerox
Copy link
Member Author

Please run pnpm run generate:locales, pnpm run generate:api-docs, and pnpm run test -u to generate/update the related files,

I did all of them and no other files where generated 🤔

@Shinigami92
Copy link
Member

I manually removed test/scripts/apidoc/temp/ and then run pnpm run test -u
Then the snapshots generated newly 🤷

/ping @ST-DDT

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #1804 (4228d44) into next (667599d) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1804      +/-   ##
==========================================
- Coverage   99.63%   99.63%   -0.01%     
==========================================
  Files        2346     2346              
  Lines      235500   235519      +19     
  Branches     1140     1138       -2     
==========================================
+ Hits       234635   234651      +16     
- Misses        843      846       +3     
  Partials       22       22              
Impacted Files Coverage Δ
src/modules/datatype/index.ts 100.00% <100.00%> (ø)
src/modules/internet/user-agent.ts 89.94% <0.00%> (-1.48%) ⬇️
src/modules/location/index.ts 98.91% <0.00%> (+0.21%) ⬆️

@ST-DDT
Copy link
Member

ST-DDT commented Feb 1, 2023

I did all of them and no other files where generated 🤔

Worked for me without issues.

@ST-DDT ST-DDT requested a review from a team February 1, 2023 23:03
@xDivisionByZerox
Copy link
Member Author

I did all of them and no other files where generated 🤔

Worked for me without issues.

I work on a windows machine. Might that be a problem somehow? Just the only thing that comes to mind right now...

@ST-DDT
Copy link
Member

ST-DDT commented Feb 1, 2023

I work on a windows machine.

I as well. Did you run pnpm run build before running pnpm run test -u?

@xDivisionByZerox xDivisionByZerox linked an issue Feb 1, 2023 that may be closed by this pull request
@xDivisionByZerox xDivisionByZerox changed the title refactor(datatype): standardize string argument refactor(datatype): standardize arguments Feb 1, 2023
@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Feb 1, 2023

I as well. Did you run pnpm run build before running pnpm run test -u?

No, I didn't 🤔
Why would that be required to update the snapshots? Vitest is run against the typescript source, no?

But thanks for the info, I'll do that next time.

@matthewmayer
Copy link
Contributor

seems weird to change the signature when the method is already deprecated in this version. If you are currently using

faker.datatype.string(42) we want you to change to faker.string.sample({length:42}) rather than faker.datatype.string({length:42}) no?

@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Feb 2, 2023

seems weird to change the signature when the method is already deprecated in this version. If you are currently using

faker.datatype.string(42) we want you to change to faker.string.sample({length:42}) rather than faker.datatype.string({length:42}) no?

Due to #1349, faker.string.* will be standardized as well in a later PR as well. I'm currently scoping related PR per module (for reviewing reasons).

@Shinigami92 Shinigami92 enabled auto-merge (squash) February 4, 2023 20:58
@Shinigami92 Shinigami92 merged commit 1b9ca01 into next Feb 4, 2023
@xDivisionByZerox xDivisionByZerox deleted the refactor/datatype/standardize-arguments branch February 11, 2023 16:23
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
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: datatype Something is referring to the datatype module p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Standardize function parameters and defaults
5 participants