feat(person): add zodiacSign()

[luciferreeves]

This PR adds the functionality of generating random zodiac signs and birthdate

New Functions:

• faker.date.birthdate() (Moved to a separate PR)
• faker.zodiac.sign()

Usage:

faker.date.birthdate():

// generates a random birthdate of a person aged between 18 and 80
console.log(faker.date.birthdate()) // 1970-12-22T05:00:00.000Z

// generates a random birthdate of a person, between two years
console.log(faker.date.birthdate({ min: 1990, max: 2000 });) // 1992-03-06T05:00:00.000Z

you can also pass a mode to the birthdate object. mode can be 'age' or 'year'

// generates a random birthdate of a person, between two years
console.log(faker.date.birthdate({ min: 1990, max: 2000, mode: 'year' });) // 1992-03-06T05:00:00.000Z

// generates a random birthdate of a person, between two ages
console.log(faker.date.birthdate({ min: 4, max: 5, mode: 'age' });) // 2017-06-08T04:00:00.000Z

faker.zodiac.sign():

// returns a random zodiac sign
console.log(faker.zodiac.sign()) // Leo

// takes optional 'birthday' parameter, if provided returns the zodiac sign for the given birthday
console.log(faker.zodiac.sign('1997-12-20')) // Sagittarius

Unit tests for both the functions added.

Sorry for the repeated commit found later. Its a refactor, but I have some zsh extensions which automatically complete commands in the terminal and it picked up on the last commit message.

Closes #175

[netlify]

✔️ Deploy Preview for vigilant-wescoff-04e480 ready!

🔨 Explore the source changes: 652f568

🔍 Inspect the deploy log: https://app.netlify.com/sites/vigilant-wescoff-04e480/deploys/61e4c13c519e350007f8a5c9

😎 Browse the preview: https://deploy-preview-182--vigilant-wescoff-04e480.netlify.app

[Shinigami92]

I will review this after 6.0.0 was released

[pkuczynski]

I would expect faker.date.birthday to have a bit different signature:

faker.date.birthday(age: number, refDate?: string | Date)
faker.date.birthday(options: { min: number, max: number }, refDate?: string | Date)

So instead of providing a year, I would rather expect to provide an age...

[luciferreeves]

I would expect faker.date.birthday to have a bit different signature:

faker.date.birthday(age: number, refDate?: string | Date)
faker.date.birthday(options: { min: number, max: number }, refDate?: string | Date)

So instead of providing a year, I would rather expect to provide an age...

I have added a mode option to the faker.date.birthday object. Please take a look at the updated description.

[ST-DDT]

Please migrate all js files to ts.

[pkuczynski]

Please migrate all js files to ts.

I think it was an accidental commit of lib/* files...

[luciferreeves]

I removed the older js files

[ST-DDT]

Have the examples files been committed intentionally?

[luciferreeves]

I didn't notice the example files were removed from the newer structure of the repo. I removed them from this PR too.

[luciferreeves]

Everything that was suggested is added. Please check

[ST-DDT]

One last part, otherwise it looks good to me.
Thanks for your contribution.
Please note that this is scheduled for 6.1 so it might take a while before this gets merged.

[luciferreeves]

Okay, everything is fixed now. Instead of datatype.number() as suggested, I used this.faker.datatype.float({ min: 0, max: 1, precision: 0.0000000000000001 }) to replicate the Math.random() behavior.

[pkuczynski]

General comment: you should separate zodiac signs and birthday into two separate PR, as they are totally independent.

[luciferreeves]

General comment: you should separate zodiac signs and birthday into two separate PR, as they are totally independent.

@pkuczynski Maybe from next time, I will target smaller issues/PRs. Too many commits have gone in this PR and it would be hard to undo those changes, split branches and redo again.

[pkuczynski]

@pkuczynski Maybe from next time, I will target smaller issues/PRs. Too many commits have gone in this PR and it would be hard to undo those changes, split branches and redo again.

You don't need to split branches. Just create a new one and copy 3 files from this PR regarding date, create new PR from them, and delete in this PR. We don't need the whole commit/review history. This way you increase a chance that one of them will be merged faster.

[luciferreeves]

I see. Although, I still fail to see the usage of 2 PRs now since this once is almost completed review and is targeted for v6.2 and won't be merged anytime soon either. A lot of PRs target multiple features/bugs and doing multiple things in the same PR is not something new. However, I will try to split the PRs if I get time.

[Shinigami92]

We might want to discuss in team if we want just zodiacSign that accepts 'western' | 'chinese' or if we want two separate methods 🤔

[ejcheng]

We might want to discuss in team if we want just zodiacSign that accepts 'western' | 'chinese' or if we want two separate methods 🤔

I would prefer just having zodiacSign that accepts 'western' | 'chinese' or 'western' | 'eastern'.

[Shinigami92]

@luciferreeves if you like to, you can already at least move the method to the person module now after a rebase

If you do not have time, please let me know and I can take this PR over

[ST-DDT]

Team decision

We will start with zodiacSign(), then we are free to introduce parameters with the specific variant.
The locale will use person.zodiac(sign).western to ensure, future extensibility.

[ST-DDT]

Done. Ready for Review.

I used faker.definitions.person.western_zodiac_sign as that has better fallback behavior over faker.definitions.person.zodiac_sign.western.

[Shinigami92]

Could you at least add german locales as well? 🙂

[ST-DDT]

Could you at least add german locales as well? 🙂

I will do that in a separate PR.