nskins / goby

Command-line role-playing game framework
MIT License
122 stars 56 forks source link

Add i18n support #106

Open pablofullana opened 7 years ago

pablofullana commented 7 years ago

Pull Request Description

Addressing #105, this PR adds internationalization support via I18n, by:

Documentation proposal

Updating Goby messages

As a Goby contributor

Customizing and exiting language translations:

  1. Edit the corresponding file under config/locales

Adding support for new language:

  1. Add a new file for the new language to be supported under config/locales
  2. Copy config/locales/en.yml content and update as required

As an application developer using Goby gem

Customizing and exiting language translations:

  1. Add a custom directory to I18n translation lookup: I18n.load_path += Dir["#{File.expand_path File.dirname(__FILE__)}/.../*.yml"]
  2. Copy the corresponding file into that directory and update as required

Adding support for new language:

  1. Add a custom directory to I18n translation lookup: I18n.load_path += Dir["#{File.expand_path File.dirname(__FILE__)}/.../*.yml"]
  2. Create a file with the supporting the language, such as:
    # .../locales/es.yml
    es:
    ...
    no_such_item: ...
  3. Copy default en.yml content from Goby into that file and update as required
  4. Set the locale: I18n.locale = :es

Finally, I think it would be wise (and actually make other developers life easier) if we place a link to the en.yml in the documentation. By doing this, they would be able to get to that file without having to browse through all the repository.

nskins commented 7 years ago

Hi @pablofullana. Here are some points to resolve/consider:

Documentation looks good; it can be placed under "Internationalization". Instead of placing a link to the en.yml file, place one to the config/locales directory, which will list all of the available translations at once.

Thanks!

nskins commented 7 years ago

Just want to let you know that you can run rspec locally and determine if the build will succeed without pushing to Github.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.1%) to 99.871% when pulling dfc732bc07150e4816b01d58c9a698cb5f7e7337 on pablofullana:add-i18n-support into 442a04fce91c6b8b694174e3546a3c221729a4de on nskins:i18n.

pablofullana commented 7 years ago

@nskins I'm getting a very odd behavior on that failing spec. Any insights?

nskins commented 7 years ago

I ran the build locally with HEAD at this commit, and it passes. The error message at that commit says that it cannot find i18n. It sounds like there's a problem with the Gemfile, goby.gemspec, or .travis.yml. First, could you please erase those last three commits? You can do so by running:

$ git reset HEAD~3

followed by

$ git stash

to remove the uncommitted changes. Unfortunately, I don't have time right now to look at it in more detail, but please feel free to push changes and review the Travis CI builds. If any commit does not work, then please erase it since I believe the aforementioned commit is the best starting point.

nskins commented 7 years ago

I believe I have fixed that issue you were struggling with in your final commits; however, this feature should work after your first 2 or 3. Not sure, but I'd be willing to merge this if we can get the build passing.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 9c40d127c68203e07586981a23841b15c765b27a on pablofullana:add-i18n-support into 4bcf2a2696696e6e5c70063738863886a74ec382 on nskins:i18n.

pablofullana commented 7 years ago

Should be good to go now.

nskins commented 7 years ago

There is still code on your branch I am not willing to merge; particularly dfc732b, 192eb24, and (maybe) c98ecfd. I am thinking the feature will work at 42541bd or c98ecfd, but I'm not sure. I may need to clean this up at some point when I have time.