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: πŸ”₯ First punt at priority option #223

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

theryansmee
Copy link
Collaborator

@theryansmee theryansmee commented Mar 16, 2022

Allow user to set length priority to length or unique (POC)

βœ… Closes: #220

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Allow user to set length priority to length or unique (POC)

βœ… Closes: #220
@theryansmee
Copy link
Collaborator Author

@NetanelBasal @shaharkazaz - This is a super sloppy implementation that I threw together at lunch today, what are your thoughts?

@NetanelBasal
Copy link
Member

Thanks @theryansmee, @shaharkazaz will go over it asap.

Copy link
Contributor

@shaharkazaz shaharkazaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸš€

packages/falso/src/lib/core/core.ts Outdated Show resolved Hide resolved
packages/falso/src/lib/core/core.ts Outdated Show resolved Hide resolved
packages/falso/src/lib/core/core.ts Outdated Show resolved Hide resolved
packages/falso/src/lib/core/core.ts Outdated Show resolved Hide resolved
Move randUniqueElement logic into fakFromArray. Simplified
fakeFromFunction while loop logic

#220
@theryansmee
Copy link
Collaborator Author

πŸš€

#223 (comment):
Yeah you are right. I'll have a think about how we can compare complex objects. With most of our entities be happy to consider it uniques as long as the id is unique.. but i'll have to look through our functions to see what other challenges we face.

I've fixed the other bits

@theryansmee
Copy link
Collaborator Author

@shaharkazaz @NetanelBasal I've still got to write tests and clean up a few bits, but I think this is mostly there now. What are your thoughts? 😊

@shaharkazaz
Copy link
Contributor

@theryansmee I'll re-review it again today πŸ‘

@theryansmee
Copy link
Collaborator Author

theryansmee commented Apr 4, 2022

@shaharkazaz I've ended up creating objectIsUnique which is then used by the other checker functions around the library. Originally, I was hoping to just have a single function that got passed around by I ended up in type hell.

That said, I think this is actually quite an elegant solution.

PS
I think once we are happy with this MR, we should do some follow up work on splitting core.ts into multiple files and ensuring that it has a decent test coverage.

PPS
I'm not sure why the build it failing. Looks like we have an issue with packages somewhere down the line.

Ryan Smee and others added 5 commits April 5, 2022 18:39
@theryansmee
Copy link
Collaborator Author

πŸ™ƒ Just seen theres been commits to core.ts so i'm going to have to look at what they are and get them integrated into my work

@NetanelBasal
Copy link
Member

Cool, thanks!

@theryansmee
Copy link
Collaborator Author

I will be fixing the conflicts and merging this later this week :)

Ryan Smee added 2 commits June 13, 2022 12:40
# Conflicts:
#	packages/falso/src/lib/core/core.ts
#	packages/falso/src/lib/text.ts
@theryansmee
Copy link
Collaborator Author

@NetanelBasal - I've merged in the latest changes but now the build is failing on the setup-node@v2 step with getCacheEntry failed: Cache service responded with 503.

Any thoughts on what might be happening?

@NetanelBasal
Copy link
Member

Nopehttps://stackoverflow.com/questions/72598852/getcacheentry-failed-cache-service-responded-with-503

Ryan Smee added 4 commits September 12, 2022 19:22
# Conflicts:
#	packages/falso/src/lib/number.ts
#	packages/falso/src/lib/octal.ts
βœ… Closes: 220
Add tests for simple array and function scenarios. Fix typo in unique
checker logic

βœ… Closes: 220
@theryansmee
Copy link
Collaborator Author

@NetanelBasal @shaharkazaz - Hi guys, sorry for the radio silence from me (Life got a little hectic). I have fixed the merge conflicts and added a few essential tests to prove priority logic works as expected with data from arrays and from functions.

I think it's ready to be reviewed again 😊

@shaharkazaz
Copy link
Contributor

@theryansmee Hi Ryan always a pleasure to hear from you 😁
I'll re-review it today-tomorrow.

@theryansmee
Copy link
Collaborator Author

@theryansmee Hi Ryan always a pleasure to hear from you 😁 I'll re-review it today-tomorrow.

I'm back for good this time! haha Cheers mate 😊

Copy link
Contributor

@shaharkazaz shaharkazaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first cycle, I'll go in-depth over the core files, these are the first things that I saw

packages/falso/src/lib/address.ts Outdated Show resolved Hide resolved
uniqueComparer: (
item: T,
items: T[],
comparisonKeys?: string[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of an object, you always pass comparisonKeys right?
If so, let's make this type stricter, and the values should also be a sub-set of T's keys.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i'm honest I was having a little trouble with the types for this. I had a lot of issues when I tried to make this not optional. I have changed the comparisonKeys type from string[] to (keyof T)[] though

packages/falso/src/lib/core/unique-validators.ts Outdated Show resolved Hide resolved
packages/falso/src/lib/core/unique-validators.ts Outdated Show resolved Hide resolved
) => boolean = (item, items) => objectIsUnique(item, items, ['id']);

export const objectIsUnique: (
item: any,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why any and not a generic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wasn't a fan of this either. On further inspection i've managed to change it to objectIsUnique<T extends Record<string, any>>. I think this should be ok for now

packages/falso/src/lib/credit-card.ts Show resolved Hide resolved
packages/falso/src/lib/flight-details.ts Show resolved Hide resolved
@@ -88,5 +96,10 @@ export function randJSON<Options extends RandomJSONOptions = never>(
return generatedObject;
};

return fake(factory, options);
return fake(factory, options, { uniqueComparer: checkUnique });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

}

const checkUnique: (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

});

describe('object id is unique', () => {
it('should return true', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Netanel's previous comment about the naming.
Maybe something like:
it(Should pass when object ids are unique) instead of the describe. This goes to all the specs I've seen in the recent files.
@NetanelBasal thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

@theryansmee
Copy link
Collaborator Author

This is the first cycle, I'll go in-depth over the core files, these are the first things that I saw

Cheers mate, i'll have a look through and get these sorted tomorrow πŸ‘

Ryan Smee and others added 4 commits September 18, 2022 14:09
Change comparison of valueOf to getTime with dateIsUnique. Combine
describe block and it labels for comparison function tests

βœ… Closes: 220
Change checkUnique arrow functions to function declorations. Change unique-validators from inline arrow functions to actual function. Make function more type safe

βœ… Closes: 220
Strengthen type checking. Remove redundant error and test

βœ… Closes: 220
Strengthen type checking for comparisonKeys

βœ… Closes: 220
@theryansmee
Copy link
Collaborator Author

@shaharkazaz - Hello mate, I have made all of the requested changes and left a few comments 😊

packages/falso/src/lib/core/core.ts Outdated Show resolved Hide resolved
config: FakeConfig<T>,
options?: Options
): T | T[] {
if (!options?.length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are handling the length options both here and in the fake function, please choose one place

const maxAttempts = options.length * 2;

while (items.length < options.length && attempts < maxAttempts) {
const item = data(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to pass the attempts as the function's argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shaharkazaz - How do you mean? To allow the user to decide the total number of attempts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are passing 0 to that factory function, I think it makes more sense to pass the attempts, if the factory is using the index passed than you will increase the chances of getting the same result.
@NetanelBasal WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

data: T[],
options?: Options
): T | T[] {
if (!options?.length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

packages/falso/src/lib/core/core.ts Outdated Show resolved Hide resolved
const newArray: T[] = [];

while (clonedData.length && newArray.length !== options.length) {
const randomIndex = getRandomInRange({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a hidden assumption here that the array you are given holds unique values.
Please change the code to support non-unique arrays.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shaharkazaz - You are correct. How did I not spot that :/

I have reworked it to shuffle the data array and then pick the first n items from it. A positive of doing it this way is that the user will receive an array with a length as close as possible to their desired length. The issue is that this will be more expensive for large data sets.

export function fakeFromArray<T, Options extends FakeOptions>(
  data: T[],
  options?: Options
): T | T[] {
  if (options?.length === 0) {
    return [];
  }

  if (!options?.length) {
    return randElement(data);
  }

  const priority = options?.priority ?? 'length';

  if (priority === 'length') {
    return Array.from({ length: options.length }, () => randElement(data));
  }

  let clonedData: T[] = JSON.parse(JSON.stringify(data));
  // Randomise order of data
  clonedData = shuffleArray<T>(clonedData);

  if (options.length >= clonedData.length) {
    return clonedData;
  }

  return clonedData.slice(0, length);
}

function shuffleArray<T>(array: T[]): T[] {
  for (let i = array.length - 1; i > 0; i--) {
    const randomIndex = Math.floor(random() * (i + 1));

    [
      array[i],
      array[randomIndex],
    ] = [
      array[randomIndex],
      array[i],
    ];
  }

  return array;
}

What are your thoughts?

Comment on lines +42 to +46
if (Array.isArray(data)) {
return fakeFromArray(data, options) as any;
}

return fakeFromFunction(data as FactoryFunction<T>, config, options) as any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a lot of shared code here, I think I would normalize the array data source to also work on a function with a random element and just use factory-based fake for both.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to do this I just felt like this whole fake logic was getting quite complex so keeping them separate made it easier to read for me. But i'm happy either way.

I think in hindsight, if we were starting over i'd suggest that instead of option.length property we have singular and plural function for each. E.G randId that returns a string & randIds that uses randId to generate & return a string[]. But it might be a bit late for that ;) haha

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I prefer the length property, adding a plural will double the API for not a good enough reason from my point of view

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair play. It works pretty well for our users, it just adds a lot of headaches for deving haha. All good though

return fake(factory, options, { uniqueComparer: checkUnique });
}

function checkUnique(card: CreditCard, cards: CreditCard[]): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is repreated several times, maybe create a factory function that creates the chacker

function uniqueByKeys(keys){
  return (a, b) => objectIsUnique(keys)
}

Copy link
Collaborator Author

@theryansmee theryansmee Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shaharkazaz - Sorry I don't know how I missed this. Do you mean something like:

export function randCreditCard<Options extends CreditCardOptions = never>(
  options?: Options
) {
  const factory: () => CreditCard = () => {
    const fullName =
      options?.fullName ??
      `${randPersonTitle()} ${randFullName({ withAccents: false })}`;
    const brand = options?.brand ?? (randCreditCardBrand() as Brand);

    const dateOptions: Intl.DateTimeFormatOptions = {
      month: 'numeric',
      year: '2-digit',
    };
    const validFrom = randPastDate({ years: 1 }).toLocaleDateString(
      'en-GB',
      dateOptions
    );
    const untilEnd = randFutureDate({ years: 2 }).toLocaleDateString(
      'en-GB',
      dateOptions
    );

    return {
      fullName,
      brand,
      validFrom,
      untilEnd,
      ccv: randCreditCardCVV(),
      number: randCreditCardNumber({ brand }),
      account: randAccount(),
      type: rand(['Credit', 'Debit']),
    };
  };

  return fake(
    factory,
    options,
    {
      // Moved logic into here rather than wrapping objectIsUnique call in new function
      uniqueComparer: (card, cards) => objectIsUnique(card, cards, ['number']) 
    }
  );
}

}

function checkUnique(item: object, items: object[]): boolean {
return items.some((i) => JSON.stringify(i) === JSON.stringify(item));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this checking the opposite? Don't you need all jsons to be different?
Looking at this as a reference:
image

}

function checkUnique(coordinate: number[], coordinates: number[][]): boolean {
return coordinates.some((c) => c.join('') === c.join(''));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as the json unique

theryansmee and others added 2 commits September 19, 2022 15:56
Change type back to original. Move option.length short circuit checks into fakeFromFunction/fakeFromArray functions. 
Fix typos in checkUnique logic for randJSON & randNearbyGPSCoordinate.
Add missing tests tests.

βœ… Closes: 220
@theryansmee
Copy link
Collaborator Author

@shaharkazaz - I have made the requested changes for all the comments that I understood and left you queries everywhere else :) I hope these all make sense

@theryansmee
Copy link
Collaborator Author

theryansmee commented Sep 27, 2022

Ok, I think all of the requested changes have been made with just a few outstanding queries left to be answered

@shaharkazaz
Copy link
Contributor

shaharkazaz commented Sep 27, 2022

@theryansmee I'm actually on vacation in Thailand πŸ˜€
I'll get back to it as soon as I return!
If @NetanelBasal is free he might pick this up

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

Successfully merging this pull request may close these issues.

Getting unique values when specifying length
3 participants