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

Ignore the "adding children to leaf" warning for keys that are discovered in source. #228

Merged
merged 2 commits into from
Feb 11, 2017

Conversation

jfly
Copy link
Contributor

@jfly jfly commented Feb 9, 2017

See #158 (comment).

I'm not very familiar with this codebase, but tests still pass!. @glebm, is this close to what you had in mind?

@glebm
Copy link
Owner

glebm commented Feb 9, 2017

@jfly Yes, this looks OK, thank you. Does this work in your case?

@jfly
Copy link
Contributor Author

jfly commented Feb 10, 2017

Ah, good question, it did not =(

I've fixed this in 4f65248, along with a test.

With the current version of i18n-tasks:

~/gitting/worldcubeassociation.org/WcaOnRails @kaladin> bundle exec i18n-tasks unused
warning: parser/current is loading parser/ruby24, which recognizes
warning: HEAD-compliant syntax, but you are running 2.4.0.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
i18n-tasks: [WARN] 'faq.answers' was a leaf, now has children (value <- scope conflict)
Unused keys (2) | i18n-tasks v0.9.11
+--------+-----------------------------+---------------------------+
| Locale | Key                         | Value                     |
+--------+-----------------------------+---------------------------+
|   ru   | registrations.events        | Дисциплины                |
|   ru   | users.edit.preferred_events | Предпочитаемые дисциплины |
+--------+-----------------------------+---------------------------+

With my changes (no warning, yay!):

~/gitting/worldcubeassociation.org/WcaOnRails @kaladin> ruby -I~/tmp/i18n-tasks/lib ~/tmp/i18n-tasks/bin/i18n-tasks unused
/usr/lib/ruby/gems/2.4.0/gems/activesupport-5.0.1/lib/active_support/xml_mini.rb:51: warning: constant ::Fixnum is deprecated
/usr/lib/ruby/gems/2.4.0/gems/activesupport-5.0.1/lib/active_support/xml_mini.rb:52: warning: constant ::Bignum is deprecated
Unused keys (2) | i18n-tasks v0.9.11
+--------+-----------------------------+---------------------------+
| Locale | Key                         | Value                     |
+--------+-----------------------------+---------------------------+
|   ru   | registrations.events        | Дисциплины                |
|   ru   | users.edit.preferred_events | Предпочитаемые дисциплины |
+--------+-----------------------------+---------------------------+

@jfly
Copy link
Contributor Author

jfly commented Feb 10, 2017

Unfortunately, it looks like rubocop failed in the last build: https://travis-ci.org/glebm/i18n-tasks/jobs/200283068#L512.

I could work around this, but I'd like to wait to hear your thoughts on the latest implementation first. If we're going to do it differently, then I don't think it's worth the effort to please rubocop right now.

@glebm
Copy link
Owner

glebm commented Feb 10, 2017

@jfly This implementation looks fine. 👍

@jfly
Copy link
Contributor Author

jfly commented Feb 10, 2017

Ok, sweet! I can easily address the line length issues, but I can't easily fix:

lib/i18n/tasks/data/tree/node.rb:13:19: C: Metrics/ParameterLists: Avoid parameter lists longer than 5 parameters.
    def initialize(key:, value: nil, data: nil, parent: nil, children: nil, warn_about_add_children_to_leaf: true)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

How would you like me to address this? Easiest fix is to change .rubocop.yml to not check this.

@glebm
Copy link
Owner

glebm commented Feb 10, 2017

Best just ignore it in that particular case, so that we still get warnings elsewhere:

# rubocop:disable Metrics/ParameterLists
def initialize(...)
end
# rubocop:enable Metrics/ParameterLists

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.

2 participants