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

Remove devise translations #2972

Closed
wants to merge 2 commits into from

Conversation

afdev82
Copy link
Contributor

@afdev82 afdev82 commented Nov 23, 2018

This PR remove devise translations, since they are also removed in the solidus_i18n project (see solidusio/solidus_i18n#107) and they are provided by the devise_i18n gem (https://github.com/tigrish/devise-i18n).

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks 👏

kennyadsl
kennyadsl previously approved these changes Nov 23, 2018
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

There is a quite a bit of noise from reformatting here, but it looks okay.

Thanks!

@kennyadsl
Copy link
Member

@afdev82 sorry, can you explain better why is the reformatting commit needed?

@afdev82
Copy link
Contributor Author

afdev82 commented Nov 26, 2018

@kennyadsl usually the translations are alphabetically ordered and so on "normalized". In this way in both projects (solidus_i18n and solidus) they are in the same place and it's easier to translate them. I would like to add the check for the health of the translations in the ci, in this way every time someone adds a translation it should be in the right place.
At the moment there will be a lot of work to get it working, I tried to check and I have to fine tuning the configuration of the i18n-tasks gem.

@kennyadsl
Copy link
Member

usually the translations are alphabetically ordered and so on "normalized".

Got that but shouldn't this be handled by i18n-tasks gem?
What I don't get is why the PR #2963 moved things around and we are reverting some of its changes here?

@afdev82
Copy link
Contributor Author

afdev82 commented Nov 26, 2018

Hm... I just run bundle exec i18n-tasks normalize inside the core directory, and then committed the result. And I did this in two commits to make it clearer, if you want we can remove the normalize commit, but it should be fine to always execute this command. If you execute bundle exec i18n-tasks health, it tells you that the translations are not normalized, then I executed that.

@kennyadsl
Copy link
Member

Yes but look at this for example. It does the opposite of the normalization happened in this PR, maybe some options changed on you env in the meantime?

@afdev82
Copy link
Contributor Author

afdev82 commented Nov 26, 2018

I see... I have to check, I don't know why now.

@kennyadsl kennyadsl dismissed their stale review November 27, 2018 15:27

Need to understand why the extra commit is needed

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

Let's investigate why bundle exec i18n-tasks normalize displayed different behaviors in this PR and #2963.

I agree we should add the health check to CI to avoid this kind of confusion in the future.

@jacobherrington jacobherrington dismissed their stale review November 27, 2018 15:42

See my above comment.

@kennyadsl
Copy link
Member

@afdev82 👋do you have any update here?

@afdev82
Copy link
Contributor Author

afdev82 commented Jan 14, 2019

No sorry, I didn't have time to continue on that :(

@kennyadsl
Copy link
Member

Reopened as #3132

@kennyadsl kennyadsl closed this Mar 5, 2019
@afdev82 afdev82 deleted the remove_devise_translations branch October 16, 2020 10:21
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