bullet_train-internationalization

[gazayas]

bullet-train-co/bullet_train#726

Namespace Collision

I originally wanted to name this bullet_train-i18n similar to devise-i18n, but we were getting a namespace collision in the resolver.

devise-i18n avoids namespace collision in their repository by using DeviseI18n instead of Devise::I18n. I ended up choosing Internationalization, and although the naming I decided to go with is longer, we can explicitly avoid the namespace collision and maintain the same structure as other gems we already have in place at the same time, so I personally think it’s a good option.

Incompatibility with devise-i18n

devise-i18n uses its own views, so although the translations are working, we're getting the same view order bug we did a while back in #33 where the original devise gem's files were taking precedence over our custom ones:

Things I would like to get done

1. Ask developers which default language they would like to use for their application in the bin/configure script.
2. Make a rake task to eject all of the locales to the starter repository for editing.
3. Make compatible with devise-i18n.

[gazayas]

We could move the locale helper here too.

[andrewculver]

@gazayas Any idea what we have to do here to get this passing CI?

[gazayas]

Will have to double check on this one, I'll try to sit down with this one again sooner than later.

[gazayas]

@andrewculver Fixed!

Since this is a joint PR, it needs bullet-train-co/bullet_train#726 and then we need to add bullet_train-internationalization to the Gemfile after it's released to work.

I saw you added a joint PR tag, so I'll start adding those from here on out.

How to test locally:

1. Fetch and checkout features/internationalization for both the starter repository and this repository.
2. Run bin/hack --link in the starter repository.
3. Point to this PR in the Gemfile like so:

gem "bullet_train-internationalization", git: '[email protected]:bullet-train-co/bullet_train-core.git', branch: 'features/internationalization', glob: 'bullet_train-internationalization/*.gemspec'

I had to test both the local gem with path: "local/bullet_train-core" and hosted gem with git: because the tests were failing due to a File.dirname issue.

[jagthedrummer]

@gazayas Can you look at the conflicts here? And also merge/rebase main? And could you push the branch for your joint PR in the starter repo directly to the starter repo so that CI can find it and use it for testing this branch?

[gazayas]

@jagthedrummer I added the following in bullet-train-co/bullet_train@625e36a to make the CI tests pass in this PR:

gem "bullet_train-internationalization", git: "[email protected]:bullet-train-co/bullet_train-core.git", branch: "features/internationalization", glob: "bullet_train-internationalization/*.gemspec"

I will go ahead and revert the change though since we should be using an officially released gem.

[gazayas]

We may have to update this to 1.3.0

[jagthedrummer]

Yeah, we should definitely update this to match the version number for everything else.

[gazayas]

@jagthedrummer I've cleaned up the PR a bit, but the only thing I'm not sure about right now is the gem versions that are in here and the Gemfile which are set explicitly. I don't think they should be, but just wanted to double check what you think about it.

[jagthedrummer]

@gazayas I think we probably want to match whatever version constraints the other core gems are using.

[gazayas]

@jagthedrummer Alright, what I've got here matches what we have in bullet_train.gemspec and bullet_train-roles.gemspec.

[jagthedrummer]

We don't need this .DS_Store file, or the one above it. Should probably add that to the .gitignore.

[jagthedrummer]

Do we need this? (I don't know a lot about rbs, but it doesn't seem like it's adopted by the project.)

[gazayas]

@jagthedrummer Thanks for the review, I went ahead and applied the changes!

[jagthedrummer]

Hey @gazayas I was just trying to get this cleaned up and ready to merge, and then realized that I don't quite understand the goal here. What's the advantage to creating a new gem to house all of the locale files vs just leaving them where they are?

[jagthedrummer]

I pulled down this branch and rebased against main to get it up to date, and when I ran into merge conflicts I'm not sure that I did the right thing on these two files. Should we be removing them? (I kinda think not...)

[jagthedrummer]

Also, just making a note so I don't forget it.

I think if we do want to move the locale file into a gem that we should do it in a multi-step process to make the merging/deployment smoother.

1. Add an empty internationalization gem
2. Publish that gem (by bumping and releasing core)
3. Update the starter repo to install that gem
4. Move locale files from their current location and into the new gem
5. Bump and release core like normal.

[gazayas]

Hey @gazayas I was just trying to get this cleaned up and ready to merge, and then realized that I don't quite understand the goal here. What's the advantage to creating a new gem to house all of the locale files vs just leaving them where they are?

@jagthedrummer When I started translating on a personal project, I ran into the devise strings and took notes from devise-i18n. I figured if they have a gem for I18n, then we probably should too for design purposes if it's a good route to take. We could potentially also extract stuff like the locale_helper in the future. I can understand why it’s not necessary to create a new gem for it though, so I'm open to how we handle this.

[jagthedrummer]

@gazayas, did the existing structure cause you any problems as you were localizing your personal project? Like was this intended to solve a specific pain you were experiencing?

It looks to me like devise-i18n isn't really a purposeful decision that the devise team made. I think it's someone who's not on their core team trying to plug a hole that they see in devise.

From the README for devise-i18n:

Devise supports i18n in controllers, models, and in other areas, but it does not have support for internationalized views. devise-i18n adds this support. Devise also does not include the actual translations. devise-i18n does this too.

If there are some real benefits to extracting a gem for this I'm definitely not opposed to it. But I'm not sure we should do it otherwise. I'm also open to persuasion.

[gazayas]

@jagthedrummer It's been a while so I don't remember any specific problems, and if we can't come up with any for the time being I'm ok with closing this one. We can at least consider it for the future if anything comes up.

[jagthedrummer]

OK, I'm gonna close it for now since there's so much other stuff in flight.