faker icon indicating copy to clipboard operation
faker copied to clipboard

feat(datatype)!: reimplement datatype module

Open Shinigami92 opened this issue 2 years ago • 13 comments
trafficstars

https://github.com/faker-js/faker/issues/1590#issuecomment-1507328948

This implements all methods that can be compared against the typeof operator

Shinigami92 avatar Apr 13 '23 20:04 Shinigami92

Also maybe dont sort the methods for this PR to reduce the diff. Separate PR.

ST-DDT avatar Apr 13 '23 21:04 ST-DDT

Codecov Report

Attention: Patch coverage is 86.45418% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 99.59%. Comparing base (9882760) to head (4fa9a18). Report is 87 commits behind head on next.

:exclamation: Current head 4fa9a18 differs from pull request most recent head 6e66d9a. Consider uploading reports for the commit 6e66d9a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2044      +/-   ##
==========================================
+ Coverage   99.55%   99.59%   +0.04%     
==========================================
  Files        2817     2801      -16     
  Lines      251203   252578    +1375     
  Branches     1125     1121       -4     
==========================================
+ Hits       250080   251562    +1482     
+ Misses       1094      989     -105     
+ Partials       29       27       -2     
Files Coverage Δ
src/modules/number/index.ts 100.00% <100.00%> (ø)
src/modules/string/index.ts 100.00% <100.00%> (ø)
src/modules/datatype/index.ts 94.48% <86.06%> (-5.52%) :arrow_down:

... and 182 files with indirect coverage changes

codecov[bot] avatar Apr 13 '23 21:04 codecov[bot]

Also maybe dont sort the methods for this PR to reduce the diff. Separate PR.

This is done explicitly We have two sections

  1. the typeof in sorted order
  2. the old deprecated methods (order not changed)

I suggest to just read/review the new code above the // deprecated methods comment everything below did not change and was not touched

Shinigami92 avatar Apr 13 '23 21:04 Shinigami92

For some of the suggestions, lets talk tomorrow when we do the v8.0 dev-weekend

Shinigami92 avatar Apr 14 '23 11:04 Shinigami92

The new implementations of these methods seem so different from the old methods that it feels confusing to keep them with the same names.

I feel if someone proposed "a method which returns a Number, but it could be NaN or POSITIVE_INFINITY so its only useful for checking with typeof" as a new issue it would be tagged as "waiting for user interest". So does it make sense to implement these very limited methods if we dont know if users actually need them?

matthewmayer avatar Apr 15 '23 14:04 matthewmayer

@ST-DDT in the meeting notes we also had defined unknown(): any of the datatype methods (TS "unknown") Can we outsource this to an issue and mark as awaiting for user interest?

Shinigami92 avatar Apr 15 '23 15:04 Shinigami92

The new implementations of these methods seem so different from the old methods that it feels confusing to keep them with the same names.

We know that, but sadly there is no other option as these methods are explicitly named for what the typeof operator supports

I feel if someone proposed "a method which returns a Number, but it could be NaN or POSITIVE_INFINITY so its only useful for checking with typeof" as a new issue it would be tagged as "waiting for user interest". So does it make sense to implement these very limited methods if we dont know if users actually need them?

Yes this was a team decision on 13. April 2023.

Reasons:

  • We dont want to drop the datatype module because we could not find another place for boolean
  • Then we came up with the idea to make the datatype module valuable for the typeof operator

Now it is possible to e.g. use faker.datatype.number() to pass to a function that just accepts a number. The test can then check that anykind of number, so also NaN or Infinity, are handled correctly by that function. This should be mostly (or even only) the case when e.g. you go into a already existing project and introduce faker. Then you can write generic tests with these datatype methods and talk to you project lead and tell them to enhance their error handling or TypeScript definitions (if they are using TypeScript at all).


Edit 1: @matthewmayer now you know the reasons behind, you are very good in pointing out to us when something is missing in the docs So it would be very welcome if you could help me with that :slightly_smiling_face:

Shinigami92 avatar Apr 16 '23 09:04 Shinigami92

Wouldn't it make the migration path much easier if new names were used for the new methods

For example faker.datatype.numberType instead of faker.datatype.number?

matthewmayer avatar Apr 16 '23 10:04 matthewmayer

Uncommitted changes were detected after runnning generate:* commands. Please run pnpm run preflight to generate/update the related files, and commit them.

github-actions[bot] avatar Feb 16 '24 23:02 github-actions[bot]

I'm thinking about to move this to v10, because in v9 the deprecated methods will be removed. In v10 we could be sure that all users are moved away from these old methods, because they where not available at all anymore.

Another idea would be to define a new module that makes the new aim more clear, like faker.typeof.number, faker.typeof.string and so on :thinking:

Shinigami92 avatar Feb 17 '24 09:02 Shinigami92

I would be in favour of a different module name.

matthewmayer avatar Feb 17 '24 09:02 matthewmayer

Although I think "typeof" is a reserved word so it would have to be something else.

matthewmayer avatar Feb 17 '24 13:02 matthewmayer

Although I think "typeof" is a reserved word so it would have to be something else.

If I would try to, TS shows: 'typeof' is not allowed as a variable declaration name. ts(1389)

Shinigami92 avatar Feb 17 '24 15:02 Shinigami92

How about faker.type.*

matthewmayer avatar Feb 21 '24 13:02 matthewmayer

This is probably no longer blocked (but needs a rebase and some thoughts).

ST-DDT avatar Mar 24 '24 15:03 ST-DDT