faker icon indicating copy to clipboard operation
faker copied to clipboard

refactor: rename helpers to helper

Open Shinigami92 opened this issue 2 years ago • 7 comments

todos:

  • [ ] needs test for deprecation message

Shinigami92 avatar May 07 '22 15:05 Shinigami92

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%) :arrow_down:
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

codecov[bot] avatar May 07 '22 15:05 codecov[bot]

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

ST-DDT avatar May 07 '22 16:05 ST-DDT

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.

Shinigami92 avatar May 07 '22 16:05 Shinigami92

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...

pkuczynski avatar May 09 '22 12:05 pkuczynski

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: https://github.com/faker-js/faker/discussions/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)

ST-DDT avatar May 09 '22 14:05 ST-DDT

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: https://github.com/faker-js/faker/discussions/805

Sure! Done here: https://github.com/faker-js/faker/discussions/805#discussioncomment-2717280 - 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...

pkuczynski avatar May 09 '22 19:05 pkuczynski

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

Shinigami92 avatar May 10 '22 19:05 Shinigami92

This got stale

Shinigami92 avatar Aug 11 '22 17:08 Shinigami92