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-unused don't remove whole files #260

Closed
vsakarov opened this issue Sep 5, 2017 · 6 comments
Closed

remove-unused don't remove whole files #260

vsakarov opened this issue Sep 5, 2017 · 6 comments
Labels
Milestone

Comments

@vsakarov
Copy link

vsakarov commented Sep 5, 2017

I use pattern_router by default and have my translations split in couple of files for different components. But when I remove a component remove-unused always leaves some translations in the file and never removes it or leaves it empty. These leftovers are then reported as unused, but still can't be removed automatically as it should remove the file, but it doesn't. Another problem that may be related is when you have file that has no mapping in write section and all its translations are unused, remove-unused will leave it intact.

@glebm
Copy link
Owner

glebm commented Sep 20, 2017

Can you demonstrate this by adding a failing integration test here?

If not, there could be a problem in your config.

@vsakarov
Copy link
Author

In order to demonstrate it I have to add new file in the fixtures which breaks a lot of tests for various reasons, but currently the relevant one is remove_unused. Here is the minimal diff I could write without modifying i18n-tasks.yaml.erb:

diff --git a/spec/i18n_tasks_spec.rb b/spec/i18n_tasks_spec.rb
index 7972c1e..c020200 100644
--- a/spec/i18n_tasks_spec.rb
+++ b/spec/i18n_tasks_spec.rb
@@ -360,6 +360,7 @@ resolved_reference_target.a (resolved ref)
 
     fs = fixtures_contents.merge(
       'config/locales/en.yml' => { 'en' => en_data }.to_yaml,
+      'config/locales/devise.en.yml' => { 'en' => { 'unused' => en_data['unused'] } }.to_yaml,
       'config/locales/es.yml' => { 'es' => es_data }.to_yaml,
       # test that our algorithms can scale to the order of {BENCH_KEYS} keys.
       'vendor/heavy.file'     => Array.new(BENCH_KEYS) { |i| "t('bench.key#{i}') " }.join

glebm added a commit that referenced this issue Sep 24, 2017
@glebm glebm added the bug label Sep 24, 2017
@glebm
Copy link
Owner

glebm commented Sep 24, 2017

Thanks for the test case! I think the commit linked above fixes it. Please try the version from master.

Another problem that may be related is when you have file that has no mapping in write section and all its translations are unused, remove-unused will leave it intact.

This seems to have been fixed by the commit above as well. I've opened (and closed) a separate issue #262 to track this in case it hasn't been fixed completely.

@glebm glebm added this to the v0.9.19 milestone Sep 24, 2017
glebm added a commit that referenced this issue Sep 24, 2017
glebm added a commit that referenced this issue Sep 24, 2017
@vsakarov
Copy link
Author

This fix is potentially dangerous as if someone adds external gem in read section of the config it may end up cleaning up gems.
Also the problem is bigger that this, I've managed to make normalize with pattern router leave old entries that become duplicates. I can open a new issue about this with tests if you like. Still not sure whether there is problem with data remove operations.

@glebm
Copy link
Owner

glebm commented Sep 24, 2017

This fix is potentially dangerous as if someone adds external gem in read section of the config it may end up cleaning up gems.

This was the behaviour before the fix as well.

I've managed to make normalize with pattern router leave old entries that become duplicates.

Yes, please open a separate issue about the other issue.

@vsakarov
Copy link
Author

You're right, thanks for fixing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants