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

Add Final Fantasy XIV #2742

Merged
merged 6 commits into from Apr 18, 2023
Merged

Add Final Fantasy XIV #2742

merged 6 commits into from Apr 18, 2023

Conversation

ghost
Copy link

@ghost ghost commented Apr 5, 2023

Summary

Adds characters, jobs, races, data centers and zones from the video game Final Fantasy XIV.

This is based on the current World of Warcraft pattern previously implemented in Faker.

Resources used (for those wanting to add/audit data):

☄️

Adds characters, jobs, races, data centers and zones from the video game Final Fantasy XIV.
@ghost ghost changed the title Atkascha ffxiv Add Final Fantasy XIV Apr 5, 2023
Copy link
Contributor

@jremes-foss jremes-foss left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@ghost
Copy link
Author

ghost commented Apr 8, 2023

One question/note, I did not make any changes to the README.md file. This would be in relation to the Generators section and adding a <details> and <summary>.

Should this be done in this PR?

(I presume not, but figured I should ask).

@stefannibrasil
Copy link
Contributor

stefannibrasil commented Apr 14, 2023

One question/note, I did not make any changes to the README.md file. This would be in relation to the Generators section and adding a <details> and <summary>.

Should this be done in this PR?

Yes! You're right. I think we probably need to update the PR templates to clarify that, thanks for pointing it out. Could you include the new generator in the README? Thanks!

There are some errors that need to be fixed first. Could you try rebasing with main?

@stefannibrasil
Copy link
Contributor

PR template updated in #2749

@ghost
Copy link
Author

ghost commented Apr 14, 2023

@stefannibrasil Updated. Thank you.

I'm unsure what steps are next (first time contributing to Faker).

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.

that's a cool addition, thanks!
Found one typo.

doc/games/final_fantasy_xiv.md Outdated Show resolved Hide resolved
games:
final_fantasy_xiv:
characters:
- Alphinaud Leveilleur
Copy link
Contributor

Choose a reason for hiding this comment

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

weird, the CI is complaining about this:

 /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/i18n-1.12.0/lib/i18n/backend/base.rb:254:in `rescue in load_yml': can not load translations from /home/runner/work/faker/faker/lib/locales/en/final_fantasy_xiv.yml: #<Psych::SyntaxError: (/home/runner/work/faker/faker/lib/locales/en/final_fantasy_xiv.yml): did not find expected '-' indicator while parsing a block collection at line 6 column 11> (I18n::InvalidLocaleData)

does the test suite work for you locally?

Copy link
Author

@ghost ghost Apr 15, 2023

Choose a reason for hiding this comment

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

Ah, I did, and thought I had before I pushed again, after adding a lot of names. Admittedly a lot of copy/paste done.

The name 'Dozol... starts with a single quote (good 'ol FFXIV naming) which threw off the YML parsing.

Most recent push corrects this locally and corrects typo in .md file as you noted.

@ghost ghost requested a review from thdaraujo April 15, 2023 01:13
@stefannibrasil
Copy link
Contributor

I'm unsure what steps are next (first time contributing to Faker).

First-time contributors need a maintainer approval to run CI. If it passes, and it's approved, the PR can be merged. I will leave this to @thdaraujo since he's been reviewing it since the beginning. Thanks!

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 fixing the CI!

@thdaraujo thdaraujo merged commit 8deca67 into faker-ruby:main Apr 18, 2023
@ghost ghost deleted the atkascha-ffxiv branch April 18, 2023 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants