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: datatype.number and number not parameter introduced #1702

Closed
wants to merge 1 commit into from

Conversation

amaster507
Copy link

Introduced not parameter for int and float number generators.

See: https://discord.com/channels/929487054990110771/929487054990110777/1058484035875258419

I had a recurring need to generate numbers with a provided parameter to signify what number I did not want generated.

To prevent endless loops in rare cases with limited runway between min and max parameters, I included a try count with a maximum of hardcoded 100 tries if the number generated is the number not wanted. If this were to occur, and error will be thrown as documented.

@amaster507 amaster507 requested a review from a team as a code owner December 30, 2022 23:51
@Shinigami92
Copy link
Member

Somehow it feels like we could try to archive the same in a different/better way
e.g. with a helpers function that could wrap such a call
This way we could have the same also for everything else, like a faker.person.firstName call, where the return value can be excluded and then a new try will be made

@amaster507
Copy link
Author

Not against that idea either. This was my first attempt at this. But if there was a way to apply it more globally that would be helpful too.

@import-brain import-brain added c: feature Request for new feature p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision m: datatype Something is referring to the datatype module m: number Something is referring to the number module labels Dec 31, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Dec 31, 2022

IMO banning a single value isnt that useful usually I omit certain values or values that match or dont match a certain criteria.

I think a dedicated method might be useful here.

E.g. faker.helpers.exclude(source: () => T, filter: T[] | Set<T> | (T) => boolean, options: {...})

Not sure about the exact signature yet.

@codecov
Copy link

codecov bot commented Dec 31, 2022

Codecov Report

Merging #1702 (458e0dd) into next (e296ff2) will decrease coverage by 0.00%.
The diff coverage is 81.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1702      +/-   ##
==========================================
- Coverage   99.64%   99.63%   -0.01%     
==========================================
  Files        2240     2240              
  Lines      240101   240160      +59     
  Branches     1068     1075       +7     
==========================================
+ Hits       239242   239287      +45     
- Misses        838      852      +14     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/number/index.ts 94.55% <77.04%> (-5.45%) ⬇️
src/modules/datatype/index.ts 100.00% <100.00%> (ø)

@Shinigami92
Copy link
Member

IMO banning a single value isnt that useful usually I omit certain values or values that match or dont match a certain criteria.

I think a dedicated method might be useful here.

E.g. faker.helpers.exclude(source: () => T, filter: T[] | Set<T> | (T) => boolean, options: {...})

Not sure about the exact signature yet.

Yes, this is leading in the right direction as equally as I thought about it with my first comment
My only concern right now would be: is this based on mersenne/seed? Theoretically it looks like it's just a function that could be used from outside faker, could it?

Currently in my head it would look something like this (not tested at all):

function exclude(source, filter, options) {
  let success = false
  let value

  do {
    value = source(options)
    success = filter(value)
  } while (!success)

  return value
}

but as you can see, this is not bound to faker / merseene / seed at all and can be used for anything in JS world

@ST-DDT
Copy link
Member

ST-DDT commented Jan 26, 2023

Team decision

We decided that we don't want functions that are not seed dependent in our library.
These function are very easy to implement yourself: #1702 (comment)

@ST-DDT ST-DDT closed this Jan 26, 2023
@ST-DDT ST-DDT added s: invalid This doesn't seem right and removed s: needs decision Needs team/maintainer decision labels Jan 26, 2023
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: datatype Something is referring to the datatype module m: number Something is referring to the number module p: 1-normal Nothing urgent s: invalid This doesn't seem right
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants