-
-
Notifications
You must be signed in to change notification settings - Fork 920
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(location): add continent
method
#3162
feat(location): add continent
method
#3162
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #3162 +/- ##
=======================================
Coverage 99.96% 99.97%
=======================================
Files 2797 2798 +1
Lines 227777 227793 +16
Branches 952 958 +6
=======================================
+ Hits 227694 227732 +38
+ Misses 83 61 -22
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note, that we (usually) require an issue first.
https://github.com/faker-js/faker/blob/next/CONTRIBUTING.md
See #3164 |
I feel this is such a harmless addition we can probably skip the usual
I can't really think of any way this api could be extended. And it's a trivial amount of data. We have spent a long time improving the code and processes. I think in various 8.x releases we can take advantage of that and add some "low-hanging fruit" methods. |
In this case it is probably fine (thats why I put the usually in parethesis). One lesson we learned for sure is that lot location stuff seems simple, but the usecases vary a lot. Some people might need only the name, others might only need the code, and some need both related to each other (e.g. countries). I did a quick google search and found this list of continent codes.
What do you/the others think? |
I've never heard about continent codes before, but it makes sense there would be some. Just one note: the two data sources above have continents like the UN interprets them, see here. This means that for example they have
I am not hooked on either; happy to change it to whatever fits best with the rest of faker's data, as long as we have a list of continents in the end. |
I have never heard of the continent codes before (or seen them in use in any software) So I think just the names are fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because country
is also just string[]
right now, I would suggest to do the same with continent
for now
…nto joscha/location/continent
I'll merge this PR since it has sufficent approvals. Thank you for contribution @joscha. Your change will be included in the upcoming version 9.1.0 of Faker. |
Thank you all for your reviews and help to get this in. |
Adds a new function to the
location
module that generates the name of a random continent.