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

feat(helpers): add rangeToNumber method and add range parameters #1486

Merged
merged 22 commits into from
Nov 21, 2022

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Oct 24, 2022

Fixes #617

  • Adds helpers.rangeToNumber(numberOrRange: number | { min: number; max: number }): number
  • And uses it where applicable

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent labels Oct 24, 2022
@ST-DDT ST-DDT requested review from a team October 24, 2022 21:17
@ST-DDT ST-DDT self-assigned this Oct 24, 2022
@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Merging #1486 (2c871df) into next (7cbeda6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #1486   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files        2216     2216           
  Lines      238845   238921   +76     
  Branches     1029     1039   +10     
=======================================
+ Hits       237983   238059   +76     
  Misses        841      841           
  Partials       21       21           
Impacted Files Coverage Δ
src/modules/helpers/index.ts 98.55% <100.00%> (+0.04%) ⬆️
src/modules/lorem/index.ts 100.00% <100.00%> (ø)
src/modules/string/index.ts 100.00% <100.00%> (ø)
src/modules/system/index.ts 100.00% <100.00%> (ø)
src/modules/word/index.ts 98.50% <100.00%> (+0.03%) ⬆️

@Shinigami92
Copy link
Member

I'm feeling a bit unconfy about this change.
Here are just my said out load thoughts but no blocker:

  • toNumber could be made internal as I do not expect that it is in any kind helpful for an end user
  • sometimes it's *count, sometimes it's length. Lets write a rule somewhere when to use which or lets unify them
  • sometimes it's only length | { min, max }, sometimes it's length | { length | min, max }. I personally prefer the second one as then as a user you can use meth({ length: 42 }) and have readable code + it's a bit more future proof if another option param gets added (by us or by user)
  • there are still parts in the code where an if-condition is needed to convert a length option into a number, I feel like this should also be handled by the helper method

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 25, 2022

  • toNumber could be made internal as I do not expect that it is in any kind helpful for an end user

Unless you want to write your own faker functions, which applies to most of our helper functions.

  • sometimes it's *count, sometimes it's length. Lets write a rule somewhere when to use which or lets unify them

I didnt touch that part in this PR. This is a task for #1349.

  • sometimes it's only length | { min, max }, sometimes it's length | { length | min, max }.

I can change that (use the range only in options).

  • there are still parts in the code where an if-condition is needed to convert a length option into a number

Could you give me an example?

@Shinigami92
Copy link
Member

  • toNumber could be made internal as I do not expect that it is in any kind helpful for an end user

Unless you want to write your own faker functions, which applies to most of our helper functions.

Isn't this the same as you requested in image module? There I will also pass the faker instance just forward to the internal called function

  • sometimes it's *count, sometimes it's length. Lets write a rule somewhere when to use which or lets unify them

I didnt touch that part in this PR. This is a task for #1349.

Ah okay, I already thought that this PR is at least a part of

  • there are still parts in the code where an if-condition is needed to convert a length option into a number

Could you give me an example?

I was referring to this part (I originally meant not from opt to num but from num to opt):

if (typeof options === 'number') {
options = {
length: options,
};
}
const length = this.faker.helpers.toNumber(options.length ?? 1);

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 25, 2022

I was referring to this part (I originally meant not from opt to num but from num to opt):

IMO that is a different feature. Maybe toOptions(...)? Not sure whether this should be done in this PR.

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 25, 2022

Unless you want to write your own faker functions, which applies to most of our helper functions.

Isn't this the same as you requested in image module? There I will also pass the faker instance just forward to the internal called function

I don't think so or I don't know what you are referring to.

If someone external wants to write his own faker method, that happens to require a range of something e.g. a custom numericAlpha(length: number | { min: number, max: number}): string method, then the user can use the toNumber function in their implementation.
Similar on how a user would use helpers.arrayElement in their custom fooType(): FooType method.

@Shinigami92
Copy link
Member

I was referring to this part (I originally meant not from opt to num but from num to opt):

IMO that is a different feature. Maybe toOptions(...)? Not sure whether this should be done in this PR.

I see, and I think this can then be solved on a more generic way, which indeed would then not fit anymore in this PR
Something like

options = toOptions(options);
const length = toNumber(options.length ?? 1);

But reading this toNumber feels now a bit like we should call it transformRangeToNumber, but thats ugly long 😕
I hope some other voices can help us with further ideas

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 3, 2022

Team decision

  • We will name the method rangeToNumber
  • Create a separate PR adding these are helper methods that can be used to create your own faker methods to the helper module jsdocs

@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Nov 3, 2022
@ST-DDT ST-DDT changed the title feat(helpers): introduce range toNumber method and use it for length parameters feat(helpers): introduce rangeToNumber method and use it for length parameters Nov 3, 2022
@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 3, 2022

Done. Ready for review.

Shinigami92
Shinigami92 previously approved these changes Nov 4, 2022
@ST-DDT ST-DDT requested a review from a team November 9, 2022 21:27
src/modules/lorem/index.ts Outdated Show resolved Hide resolved
src/modules/lorem/index.ts Outdated Show resolved Hide resolved
Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>
import-brain
import-brain previously approved these changes Nov 20, 2022
Shinigami92
Shinigami92 previously approved these changes Nov 21, 2022
@ST-DDT ST-DDT added the m: helpers Something is referring to the helpers module label Nov 21, 2022
@ST-DDT ST-DDT changed the title feat(helpers): introduce rangeToNumber method and use it for length parameters feat(helpers): add rangeToNumber method and add range parameters Nov 21, 2022
@ST-DDT ST-DDT merged commit 9cd716e into next Nov 21, 2022
@ST-DDT ST-DDT deleted the feat/helpers/toNumber branch November 21, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: helpers Something is referring to the helpers 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.

Improve methods with a length: number argument to support ranges
4 participants