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 i18n support #106

Open
wants to merge 6 commits into
base: i18n
Choose a base branch
from
Open

Conversation

pablofullana
Copy link

@pablofullana pablofullana commented May 28, 2017

Pull Request Description

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

  • Adding i18n as a dependency
  • Adding 'lib/goby/config/locales' directory (where I18n will load translations from by default)
  • Adding a basic en.yml to be updated as required
  • Placing an example on how to set translations in Entity class

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: ...
  1. Copy default en.yml content from Goby into that file and update as required
  2. 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
Copy link
Owner

nskins commented May 28, 2017

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

  • Ensure that it defaults to English language (maybe it already does?).
  • If you know another language, place one corresponding example for the same two strings from en.yml.
  • The Travis CI build is failing; please look into that.
  • Make the pull request to branch i18n instead of master. If I recall correctly, you may need to re-open the pull request to change the base branch.

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!

@pablofullana pablofullana changed the base branch from master to i18n May 28, 2017 14:51
@nskins
Copy link
Owner

nskins commented May 28, 2017

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

@coveralls
Copy link

Coverage Status

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

@pablofullana
Copy link
Author

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

@nskins
Copy link
Owner

nskins commented May 28, 2017

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.

Repository owner deleted a comment from coveralls Aug 25, 2017
@nskins
Copy link
Owner

nskins commented Aug 25, 2017

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.

Repository owner deleted a comment from coveralls Aug 25, 2017
@coveralls
Copy link

Coverage Status

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

@pablofullana
Copy link
Author

Should be good to go now.

@nskins
Copy link
Owner

nskins commented Aug 26, 2017

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.

spec.authors = ["Nicholas Skinsacos"]
spec.email = ["[email protected]"]

spec.summary = %q{Framework for creating text RPGs.}
spec.homepage = "https://github.com/nskins/goby"
spec.license = "MIT"

spec.files = Dir["{lib}/**/*", "LICENSE", "README.md" ]
Copy link

Choose a reason for hiding this comment

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

This line is redundant, it's re-specified next line down.

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