feat(zodiac): add new module

[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[bot]]

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

🔨 Explore the source changes: 652f568df78e39b1fc2dd45b6e942cbd60644f44

🔍 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

[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.

[ST-DDT]

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.

Can you explain why? AFAIK datatype.number should return a value between startDate and endDate.

[ST-DDT]

I replaced your return statement with

return new Date(
      this.faker.datatype.number({
        min: startDate,
        max: endDate,
      })
    );

And the tests still pass.

[luciferreeves]

Okay, done. I used:

return new Date(
  this.faker.datatype.number({
    min: startDate,
    max: endDate,
  })
);

[luciferreeves]

I have resolved all the notified issues, and added comments to the unresolved ones.

[luciferreeves]

Tests pass on my system but are failing here. I am looking into it. Please disregard the review request.

[ST-DDT]

You might have to rebase.

[luciferreeves]

Okay, I think this is now ready to be reviewed.

[codecov[bot]]

Codecov Report

Merging #182 (8509fbe) into next (d363258) will decrease coverage by 0.00%. The diff coverage is 93.75%.

:exclamation: Current head 8509fbe differs from pull request most recent head 78878b0. Consider uploading reports for the commit 78878b0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             next     #182      +/-   ##
==========================================
- Coverage   99.64%   99.63%   -0.01%     
==========================================
  Files        2213     2176      -37     
  Lines      238637   237668     -969     
  Branches     1014     1014              
==========================================
- Hits       237783   236812     -971     
- Misses        833      835       +2     
  Partials       21       21              

Impacted Files	Coverage Δ
src/definitions/person.ts	0.00% <0.00%> (ø)
src/locales/en/person/index.ts	100.00% <100.00%> (ø)
src/locales/en/person/western_zodiac_sign.ts	100.00% <100.00%> (ø)
src/modules/person/index.ts	100.00% <100.00%> (ø)
src/locales/index.ts	100.00% <0.00%> (ø)
src/modules/git/index.ts	100.00% <0.00%> (ø)
src/locales/dv/company/adjective.ts
src/locales/dv/location/city_suffix.ts
src/locales/dv/location/postcode.ts
src/locales/dv/cell_phone/formats.ts
... and 34 more

[ST-DDT]

Could you please also include the source for the zodiac sign dates above the switch case? There seems to be different dates based on the page you visit (e.g. German wiki vs English wiki). 😱

[Shinigami92]

We're starting work on the v6.2 milestone, but this PR needs a rebase. Please rebase and resolve all conflicts. Until then, we're moving it to the v6 milestone, which now serves as the "v6-Backlog". If this PR no longer contains any conflicts and can certainly be a target for the next release, we will move it to the then active milestone v6.x.

[ST-DDT]

I updated the branch and will fix remaining issues.

[ST-DDT]

Docs Preview (Click to expand)

[Shinigami92]

Please check whether that FakerError is generally unsafe to use or just from inside our tests.

at least in the current version (v6.3.1) it is possible 🤔

if you like you could add an extra test that checks importing the error, but I trust esbuild 🤷

[ST-DDT]

if you like you could add an extra test that checks importing the error, but I trust esbuild

It might be only an issue if we use it from the faker modules itself (circular reference: module -> [email protected] -> faker -> module).

[ST-DDT]

@faker-js/members @faker-js/maintainers: As per @Shinigami92's request: Should we have faker.zodiac.sign(birthday) as option or is that too much of a utility method. (We would still have faker.zodiac.sign())

[import-brain]

@faker-js/members @faker-js/maintainers: As per @Shinigami92's request: Should we have faker.zodiac.sign(birthday) as option or is that too much of a utility method. (We would still have faker.zodiac.sign())

Hmm, I don't know. I'm leaning towards it being too much of a utility method, but let's see what the other members of the team think.