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.

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

Merged
merged 15 commits into from
Nov 8, 2022
Merged

Conversation

ZionMiller
Copy link
Contributor

@ZionMiller ZionMiller commented Oct 22, 2022

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

    def name(size, region)
      fetch("airport.#{region}.#{size}")
    rescue I18n::MissingTranslationData
      p 'valid arguments are size && region -> US has size large medium small, EU has size large medium -- united_states || european_union'
    end

Produces random US Airport by name.

@return [String]

@example
Faker::Travel::Airport.name('large', 'united_states') => "Los Angeles International Airport"

@faker.version next

…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
Copy link
Contributor

@stefannibrasil stefannibrasil left a 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 🤔

test/faker/travel/test_faker_airports.rb Outdated Show resolved Hide resolved
Co-authored-by: Stefanni Brasil <stefannibrasil@gmail.com>
@ZionMiller ZionMiller closed this Nov 1, 2022
@ZionMiller ZionMiller reopened this Nov 1, 2022
Copy link
Contributor

@thdaraujo thdaraujo 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 adding this!

There are some issues with the documentation and I left a few suggestions.

lib/faker/travel/airport.rb Outdated Show resolved Hide resolved
lib/faker/travel/airport.rb Outdated Show resolved Hide resolved
lib/faker/travel/airport.rb Outdated Show resolved Hide resolved
@thdaraujo
Copy link
Contributor

thdaraujo commented Nov 1, 2022

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.

@ZionMiller @stefannibrasil

ZionMiller and others added 3 commits November 1, 2022 09:52
Co-authored-by: Thiago Araujo <thd.araujo@gmail.com>
Co-authored-by: Thiago Araujo <thd.araujo@gmail.com>
@stefannibrasil
Copy link
Contributor

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.

@ZionMiller @stefannibrasil

I like the idea!

@ZionMiller
Copy link
Contributor Author

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.
@ZionMiller @stefannibrasil

I like the idea!

Great idea!

Copy link
Contributor

@stefannibrasil stefannibrasil left a 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
@ZionMiller
Copy link
Contributor Author

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 🌟

Added that in and updated README 🤝

Copy link
Contributor

@stefannibrasil stefannibrasil left a 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!

Comment on lines +241 to +243
### Travel
- [Faker:Travel::Airport](doc/travel/airport.md)

Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@stefannibrasil stefannibrasil merged commit 25d45f6 into faker-ruby:main Nov 8, 2022
@thdaraujo
Copy link
Contributor

thdaraujo commented Nov 18, 2022

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

@stefannibrasil
Copy link
Contributor

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!

stefannibrasil pushed a commit that referenced this pull request Nov 19, 2022
@stefannibrasil
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants