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

Getting unique values when specifying length #220

Open
juliendelort opened this issue Mar 9, 2022 · 14 comments · May be fixed by #223
Open

Getting unique values when specifying length #220

juliendelort opened this issue Mar 9, 2022 · 14 comments · May be fixed by #223
Assignees
Labels
enhancement New feature or request

Comments

@juliendelort
Copy link

Description

Currently, when specifying a length, duplicates can be returned.

Here is an example call:

randCat({ length: 10 });

result:

["Thai","LaPerm","Bombay","Siberian","Himalayan","Himalayan","Himalayan","Ocicat","Thai","Serengeti"]

Is there a way to only get unique results?

Proposed solution

Maybe an extra option:

randCat({ length: 10, unique: true });

Alternatives considered

No response

Do you want to create a pull request?

No

@Pranomvignesh
Copy link
Contributor

Hi @juliendelort , I have a doubt in this implementation.
What if the length provided in the rand function is greater than the available number of entries in the data ?

  1. If unique:true is prioritized, then the output will have less number of entries than specified in length
  2. If length is prioritized, then the output will not be unique

Either way, the function is not returning an output which is compliant to the specified input options

@juliendelort
Copy link
Author

That makes sense, thank you for the reply!
How about using a more explicit option (like tryUniques: 10 for example) and letting the user choose between strategies #1 and #2 above using another option (like priority: 'length' | 'unicity'):

// Returns 10 items for sure with possible duplicates (duplicates only if there are less than 10 entries in the data)
randCat({ tryUniques: 10, priority: 'length' }); 
// If there are less than 10 entries in the data, the below would just return all the data entries (less than 10)
randCat({ tryUniques: 10, priority: 'unicity' });

(Different option names can be used)

@NetanelBasal
Copy link
Member

@shaharkazaz @theryansmee any thoughts?

@theryansmee
Copy link
Collaborator

I really like the idea of ensuring data is unique, and I think the priority is a really interesting concept.

We need to think about:

  • How this param will work with randomly generated content rather than JSON data (Credit card number etc)
  • How we clearly document to a user that if the select unicity their array length may not be what they initially expecting. And the returned length could increase within a minor release if we add more items to a JSON file (Not that that's a massive deal)
  • Should priority just work with the length param that already exists

All that said, i've already run into issues in my unit tests with duplicated data being returned from Falso, so this would be a blessing for me 😂

@theryansmee
Copy link
Collaborator

@shaharkazaz @NetanelBasal - Did you guys have any more thoughts on this? The more I think about it, the more I think it would be cool

@NetanelBasal
Copy link
Member

I'm ok with the proposed API. It should be easy because we have the proxy function.

@theryansmee
Copy link
Collaborator

I'm going to have a little crack at it then :)

@theryansmee theryansmee self-assigned this Mar 16, 2022
theryansmee pushed a commit that referenced this issue Mar 16, 2022
Allow user to set length priority to length or unique (POC)

✅ Closes: #220
@theryansmee theryansmee linked a pull request Mar 16, 2022 that will close this issue
3 tasks
theryansmee added a commit that referenced this issue Mar 22, 2022
Move randUniqueElement logic into fakFromArray. Simplified
fakeFromFunction while loop logic

#220
theryansmee added a commit that referenced this issue Mar 22, 2022
theryansmee added a commit that referenced this issue Mar 23, 2022
Add default comparison functions for most likely use cases. Implimented
them

✅ Closes: #220
theryansmee pushed a commit that referenced this issue Apr 4, 2022
Add comparison function and start change over to using it

✅ Closes: #220
theryansmee pushed a commit that referenced this issue Apr 5, 2022
Fix snapshot error. Ensure all thrown errors throw actual errors rather
than strings

✅ Closes: #220
theryansmee pushed a commit that referenced this issue Apr 5, 2022
Add missing priority doc comment

✅ Closes: #220
theryansmee pushed a commit that referenced this issue Apr 19, 2022
theryansmee pushed a commit that referenced this issue May 3, 2022
Move validators to own file. Rename validators to follow similar naming
convention

✅ Closes: #220
theryansmee pushed a commit that referenced this issue May 6, 2022
(Still need to do tests for objectIsUnique)

✅ Closes: #220
theryansmee pushed a commit that referenced this issue May 6, 2022
theryansmee pushed a commit that referenced this issue May 10, 2022
Closes: #220

feat: 🔥 Combine uniqueComparer and comparisonKeys into config

✅ Closes: #220
theryansmee pushed a commit that referenced this issue May 12, 2022
theryansmee pushed a commit that referenced this issue May 12, 2022
theryansmee pushed a commit that referenced this issue May 12, 2022
Closes: #220

feat: 🔥 Revert package.lock

✅ Closes: #220
@NetanelBasal NetanelBasal added the enhancement New feature or request label May 21, 2022
theryansmee pushed a commit that referenced this issue Jun 13, 2022
@ollebergkvist
Copy link

Any ETA for this feature to be implemented?

@NetanelBasal
Copy link
Member

It depends on @theryansmee :)

@theryansmee
Copy link
Collaborator

It depends on @theryansmee :)

Hi. Sorry, i've not looked at this in months. Last time I looked there was a butt load of merge conflicts with my PR and I haven't had any time to solve them. I will try and see if I can get them fixed this week so we can finally get this work in.

Sorry again for the radio silence

@kfrederix
Copy link

Hello, any chance of giving this one some new love? I see a lot of effort has been done on the PR last year by @theryansmee , would be a shame to waste that i.m.o. Anything I can do to help?

@shaharkazaz
Copy link
Contributor

@kfrederix yes, I'll take a look at it this week 👍

@Hypenate
Copy link

Hypenate commented Aug 9, 2023

Hopefully we can close this, it would really come in handy!

@IO-Izm
Copy link

IO-Izm commented Sep 5, 2023

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants