Skip to content

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

Merged
glebm merged 2 commits into
glebm:masterfrom
jfly:issue-158
Feb 11, 2017
Merged

Ignore the "adding children to leaf" warning for keys that are discovered in source.#228
glebm merged 2 commits into
glebm:masterfrom
jfly:issue-158

Conversation

@jfly

@jfly jfly commented Feb 9, 2017

Copy link
Copy Markdown
Contributor

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

glebm commented Feb 9, 2017

Copy link
Copy Markdown
Owner

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

@jfly

jfly commented Feb 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

jfly commented Feb 10, 2017

Copy link
Copy Markdown
Contributor Author

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

glebm commented Feb 10, 2017

Copy link
Copy Markdown
Owner

@jfly This implementation looks fine. 👍

@jfly

jfly commented Feb 10, 2017

Copy link
Copy Markdown
Contributor Author

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

glebm commented Feb 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Owner

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

@glebm glebm merged commit 225cee6 into glebm:master Feb 11, 2017
@glebm glebm modified the milestone: v0.9.12 Feb 11, 2017
@jfly jfly mentioned this pull request Feb 12, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.9.12

Development

Successfully merging this pull request may close these issues.

2 participants

@jfly @glebm