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.

Sign up for GitHub

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

Jump to bottom

feat: new plant module #2126

Closed
wants to merge 2 commits into from
Closed

feat: new plant module #2126

wants to merge 2 commits into from

Conversation

sarahtang7
Copy link

Adds faker.plant.tree and faker.plant.flower as described in #2022

Changes made:

  • Added a new module, plant
  • Added two new methods, plant.tree and plant.flower that return random tree and flower names
  • Added data for tree and flower names
  • Updated en locale data
  • Added fixed seeded test class plant.spec.ts and snapshots

@sarahtang7 sarahtang7 requested a review from a team May 5, 2023 00:09
@sarahtang7 sarahtang7 requested a review from a team as a code owner May 5, 2023 00:09
matthewmayer
matthewmayer requested changes May 5, 2023
View reviewed changes
src/locales/en/plant/tree.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

For the trees, i think it would be better to keep the names to simpler, mostly one-word tree names. Then those could be used in other patterns like https://github.com/faker-js/faker/blob/next/src/locales/en/location/street_pattern.ts

For example "Pine Street" or "Ash Drive" is realistic, but "Lavender Twist Redbud Avenue" is not.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should have a separate method/option for simple names?

@codecov
Copy link

codecov bot commented May 5, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.55172% with 12 lines in your changes missing coverage. Please review.

Project coverage is 99.58%. Comparing base (eb2b18b) to head (ce0931c).
Report is 411 commits behind head on next.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #2126      +/-   ##
==========================================
- Coverage   99.59%   99.58%   -0.02%     
==========================================
  Files        2823     2828       +5     
  Lines      255523   255871     +348     
  Branches     1106     1110       +4     
==========================================
+ Hits       254488   254808     +320     
- Misses       1007     1034      +27     
- Partials       28       29       +1
Files Coverage Δ
src/faker.ts 89.18% <100.00%> (+0.05%) ⬆️
src/index.ts 100.00% <100.00%> (ø)
src/locales/en/index.ts 100.00% <100.00%> (ø)
src/locales/en/plant/flower.ts 100.00% <100.00%> (ø)
src/locales/en/plant/index.ts 100.00% <100.00%> (ø)
src/locales/en/plant/tree.ts 100.00% <100.00%> (ø)
src/modules/plant/index.ts 100.00% <100.00%> (ø)
src/definitions/index.ts 0.00% <0.00%> (ø)
src/definitions/definitions.ts 0.00% <0.00%> (ø)
src/definitions/plant.ts 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

ST-DDT
ST-DDT reviewed May 5, 2023
View reviewed changes
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Please note that we are approaching the v8.0 release and this PR likely wont make it in there.

Please run pnpm run preflight.

src/index.ts
@@ -54,6 +54,9 @@ export type {
PhoneNumberDefinition,
/** @deprecated Use PhoneNumberDefinition instead */
PhoneNumberDefinition as PhoneNumberDefinitions,
PlantDefinition,
/** @deprecated Use AnimalDefinition instead */
PlantDefinition as PlantDefinitions,
Copy link
Member

Choose a reason for hiding this comment

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

This is redundant as this is a new definition that didnt exist before.

src/modules/plant/index.ts
const definition = this.faker.helpers.arrayElement(
this.faker.definitions.plant.tree
);
return this.faker.helpers.fake(definition);
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to call fake() on them since the names dont have templates attached to them.

src/modules/plant/index.ts
const definition = this.faker.helpers.arrayElement(
this.faker.definitions.plant.flower
);
return this.faker.helpers.fake(definition);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@ST-DDT ST-DDT added this to the vFuture milestone May 5, 2023
@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent s: waiting for user interest Waiting for more users interested in this feature labels May 5, 2023
@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label May 5, 2023
@ST-DDT ST-DDT linked an issue May 5, 2023 that may be closed by this pull request
faker.plant.tree and faker.plant.flower #2022
Open
@ST-DDT
Copy link
Member

ST-DDT commented May 5, 2023

I also noticed the issue is marked as waiting for user interest.
It currently does not have enough up votes for immediate inclusion.

@matthewmayer
Copy link
Contributor

I think a new contributor making a pull request should count as 5 upvotes 🤣

@ST-DDT
Copy link
Member

ST-DDT commented Oct 7, 2023

It looks like some lint errors have creeped in over time.

@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Apr 4, 2024
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Jun 17, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Jun 20, 2024

Close as stale. We can reopen this when required/updated.

@ST-DDT ST-DDT closed this Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT left review comments

@matthewmayer matthewmayer matthewmayer requested changes

Assignees

@sarahtang7 sarahtang7

Labels
c: feature Request for new feature needs rebase There is a merge conflict p: 1-normal Nothing urgent s: waiting for user interest Waiting for more users interested in this feature
Projects
No open projects
Faker Roadmap
Status: Awaiting Review
Milestone
vFuture
Development

Successfully merging this pull request may close these issues.

faker.plant.tree and faker.plant.flower
3 participants
@sarahtang7 @ST-DDT @matthewmayer