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: rename helpers to helper #933

Closed
wants to merge 1 commit into from

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented May 7, 2022

todos:

  • needs test for deprecation message

@Shinigami92 Shinigami92 added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs labels May 7, 2022
@Shinigami92 Shinigami92 self-assigned this May 7, 2022
@codecov
Copy link

codecov bot commented May 7, 2022

Codecov Report

Merging #933 (88e88a2) into main (8189b93) will increase coverage by 0.00%.
The diff coverage is 98.11%.

@@           Coverage Diff           @@
##             main     #933   +/-   ##
=======================================
  Coverage   99.66%   99.66%           
=======================================
  Files        1957     1957           
  Lines      209831   209831           
  Branches      891      890    -1     
=======================================
+ Hits       209121   209134   +13     
+ Misses        690      678   -12     
+ Partials       20       19    -1     
Impacted Files Coverage Δ
src/definitions/finance.ts 0.00% <0.00%> (ø)
src/definitions/hacker.ts 0.00% <0.00%> (ø)
src/definitions/phone_number.ts 0.00% <0.00%> (ø)
src/modules/image/providers/lorempixel.ts 92.01% <0.00%> (ø)
src/faker.ts 100.00% <100.00%> (ø)
src/modules/address/index.ts 99.79% <100.00%> (-0.01%) ⬇️
src/modules/animal/index.ts 100.00% <100.00%> (ø)
src/modules/commerce/index.ts 100.00% <100.00%> (ø)
src/modules/company/index.ts 100.00% <100.00%> (ø)
src/modules/database/index.ts 100.00% <100.00%> (ø)
... and 19 more

@Shinigami92 Shinigami92 requested review from ST-DDT and a team May 7, 2022 15:55
@ST-DDT
Copy link
Member

ST-DDT commented May 7, 2022

I like helpers more than helper.
If you want to use singular names everywhere, then maybe use util? (It is as generic as helpers)

@Shinigami92
Copy link
Member Author

I like helpers more than helper. If you want to use singular names everywhere, then maybe use util? (It is as generic as helpers)

I had the exact same thought! 🤔
Let's ask other members or put this on meeting notes.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label May 7, 2022
@pkuczynski
Copy link
Member

I don't like both names :) We should keep the whole thing private and used only internally, move things like replaceCreditCardSymbols closer to the usage and maybe expose some (arrayElement) in faker.random... Faker is meant to generate data and not provide utilities to replaces characters in strings - so those utility functions do not belong to it I think...

@ST-DDT
Copy link
Member

ST-DDT commented May 9, 2022

I don't like both names :)

Do you have an alternative? Is util at least better or worse than helper(s)?

We should keep the whole thing private

Not sure about that. Some of the methods maybe, but I assume some will be left in (t)here.

move things like replaceCreditCardSymbols closer to the usage

Yes, some of them could be moved. Some even to the potential new strings module.

If you have a specific suggestion, consider opening an issue/PR or document your idea here:
#805

maybe expose some (arrayElement) in faker.random

Well we have to expose them somewhere. Whether as helpers, util or random isn't that important.
However random is once again really generic. I think util is the better name for those though.
(Assuming the original random will be split and purged eventually.)

Faker is meant to generate data and not provide utilities to replaces characters in strings - so those utility functions do not belong to it I think...

I agree to a certain extend to it. However,

  • if we need the methods for our library ourselves,
  • or they are fundamentally easy and useful in their nature, so that they are worth maintaining

then we should retain and consider keeping them public. (But that also true for the other modules and one of the reasons, why I'm kind of against #883)

@pkuczynski
Copy link
Member

Do you have an alternative? Is util at least better or worse than helper(s)?

Don't really see much different between those 2 (3) to be honest.

If you have a specific suggestion, consider opening an issue/PR or document your idea here:
#805

Sure! Done here: #805 (comment) - once it will get our consensus, I can work on PRs :) It does not make sense to do that if we won't agree on it...

However random is once again really generic. I think util is the better name for those though.
(Assuming the original random will be split and purged eventually.)

I think random is a good name and can be described as picking up a random value from collection (see my proposal of random.element or random.order - that reads very well).

Stuff currently in random should go away to more dedicated modules, as those methods generate data, while random could be only picking data from given collection.

then we should retain and consider keeping them public

I listed in #805 what I think make sense to keep public and what not. Lets discuss this over there...

As of now renaming helpers to helper or util brings no major value from my pov...

@Shinigami92 Shinigami92 added do NOT merge yet Do not merge this PR into the target branch yet s: on hold Blocked by something or frozen to avoid conflicts labels May 10, 2022
@Shinigami92
Copy link
Member Author

As in v6 we currently marked things like random.arrayElement to be moved to helpers.arrayElement, I would suggest we will make the renaming / moval of helpers in v7->v8
Additionally we should write a complete and final solution down what we want to move where and make final decisions so we do not move stuff again and again

@xDivisionByZerox xDivisionByZerox added the m: helpers Something is referring to the helpers module label Jul 28, 2022
@import-brain import-brain added the deprecation A deprecation was made in the PR label Aug 11, 2022
@Shinigami92
Copy link
Member Author

This got stale

@Shinigami92 Shinigami92 deleted the rename-helpers-to-helper branch August 11, 2022 17:15
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 deprecation A deprecation was made in the PR do NOT merge yet Do not merge this PR into the target branch yet m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision s: on hold Blocked by something or frozen to avoid conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants