-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
added travel folder and airports #2601
Conversation
…seeds and would love the addition to Faker to help save time on travel apps. This commit contains us and eu airports by size and iata code
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.
Thank you for your contribution!
The PR body is a being previewed weirdly. Would you mind editing it to be a paragraph?
Could you please remove the comment in the test file? It's not really adding anything 🤔
Co-authored-by: Stefanni Brasil <stefannibrasil@gmail.com>
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.
thanks for adding this!
There are some issues with the documentation and I left a few suggestions.
what do you think of passing the continent/region as an argument to the generator? the reason is that if we decide to add more regions and airports from countries outside of US and Europe in the future, we would have to add new generators as well, probably one for each region. |
Co-authored-by: Thiago Araujo <thd.araujo@gmail.com>
Co-authored-by: Thiago Araujo <thd.araujo@gmail.com>
I like the idea! |
Great idea! |
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.
Looking great!
What @thdaraujo was suggesting it to pass the continent/region as an argument.
I think we could even differentiate between generating the name or the IATA code:
Faker::Travel::Airport.name(size: 'small', region: 'north_america') => "Palm Springs International Airport"
Faker::Travel::Airport.iata(size: 'large', region: 'europe') => "FRA"
Also, could you add the new generator to the README? That way, new users can find this new generator and know how to use it 🌟
…size && region, provided some rescue for method, and added generator to README
…size && region, provided some rescue for method, and added generator to README
Added that in and updated README 🤝 |
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.
Awesome. Thank you for your contribution to faker!
### Travel | ||
- [Faker:Travel::Airport](doc/travel/airport.md) | ||
|
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.
💯
thanks for the contribution, @ZionMiller! I'm a bit late, but going back to the original suggestion from @stefannibrasil here, I'm wondering if it would be better for this generator to have keyword arguments instead of positional ones. What do you think? The reason is that we just moved away from positional arguments on faker v3. I think it would make sense to have it as a standard for new generators too. What do you think? @stefannibrasil @ZionMiller |
ohhh, yes! We totally missed this detail. I'm going to revert this PR. We don't support positional arguments anymore since v3. @ZionMiller could you make that change and update the docs with the keywords arguments? Thank you! |
This reverts commit 25d45f6.
Could you open a new PR wit this new change? Sorry for the confusion, I didn't catch that during the review. Let me know if you have any questions @ZionMiller Thank you! |
Summary
I often find myself creating these seeds and would love the addition to Faker to help save time on travel apps. This pull contains US and EU airports by size and IATA code, allowing the community to generate random airports to create tickets etc for travel apps.
Other Information
Produces random US Airport by name.
@return [String]
@example
Faker::Travel::Airport.name('large', 'united_states') => "Los Angeles International Airport"
@faker.version next