Skip to content

Silence warnings for common leaf -> tree expansion scenario#314

Merged
glebm merged 1 commit into
glebm:masterfrom
aleksandrs-ledovskis:feature/skip-warnings-for-pluralization-branch-outs
Dec 24, 2018
Merged

Silence warnings for common leaf -> tree expansion scenario#314
glebm merged 1 commit into
glebm:masterfrom
aleksandrs-ledovskis:feature/skip-warnings-for-pluralization-branch-outs

Conversation

@aleksandrs-ledovskis

@aleksandrs-ledovskis aleksandrs-ledovskis commented Oct 27, 2018

Copy link
Copy Markdown
Contributor

Closes #242

@aleksandrs-ledovskis aleksandrs-ledovskis force-pushed the feature/skip-warnings-for-pluralization-branch-outs branch from 6083dd8 to 7131653 Compare October 27, 2018 12:48
Gargron
Gargron reviewed Oct 27, 2018
View reviewed changes
Comment thread lib/i18n/tasks/data/tree/siblings.rb Outdated
# siblings' keys are unique
class Siblings < Nodes # rubocop:disable Metrics/ClassLength
# Ref: http://cldr.unicode.org/index/cldr-spec/plural-rules
CLDR_CATEGORY_KEYS = %w[zero one two few many other].freeze

@Gargron Gargron Oct 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could maybe reuse a similar definition from I18n::Tasks::PluralKeys

@aleksandrs-ledovskis aleksandrs-ledovskis Oct 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactored

@aleksandrs-ledovskis aleksandrs-ledovskis force-pushed the feature/skip-warnings-for-pluralization-branch-outs branch 2 times, most recently from dc5dc71 to a22e6b1 Compare October 28, 2018 10:20
@aleksandrs-ledovskis aleksandrs-ledovskis force-pushed the feature/skip-warnings-for-pluralization-branch-outs branch 2 times, most recently from 81eb609 to 774c3d6 Compare November 4, 2018 17:52
@aleksandrs-ledovskis

aleksandrs-ledovskis commented Nov 4, 2018

Copy link
Copy Markdown
Contributor Author

Build is green + rebased on latest master

glebm
glebm reviewed Nov 4, 2018
View reviewed changes
Comment thread lib/i18n/tasks/data/tree/siblings.rb Outdated
def conditionally_warn_add_children_to_leaf(node, children)
return unless @warn_about_add_children_to_leaf
return if !children.empty? &&
(children.map(&:key) - I18n::Tasks::PluralKeys::CLDR_CATEGORY_KEYS).empty?

@glebm glebm Nov 4, 2018

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Consider exposing and reusing the following method here:

i18n-tasks/lib/i18n/tasks/plural_keys.rb

Line 50 in 774c3d6

def plural_forms?(s)

@aleksandrs-ledovskis aleksandrs-ledovskis Nov 5, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion! Refactored and added extra test to cover "internal node" situation

@aleksandrs-ledovskis aleksandrs-ledovskis force-pushed the feature/skip-warnings-for-pluralization-branch-outs branch from 774c3d6 to bc5673a Compare November 5, 2018 20:14
@aleksandrs-ledovskis
Silence warnings for common leaf -> tree expansion scenario.
a771f4c 
Closes glebm#242

Signed-off-by: Aleksandrs Ļedovskis <aleksandrs@ledovskis.lv>
@aleksandrs-ledovskis aleksandrs-ledovskis force-pushed the feature/skip-warnings-for-pluralization-branch-outs branch from bc5673a to a771f4c Compare December 24, 2018 07:42
@aleksandrs-ledovskis

aleksandrs-ledovskis commented Dec 24, 2018

Copy link
Copy Markdown
Contributor Author

@glebm Hello. The review's changes were applied, branch is rebased and build passes (JRuby failures are from upstream - ruby-i18n/i18n#447). Seems LGTM?

@glebm glebm merged commit b59b1fb into glebm:master Dec 24, 2018
@glebm

glebm commented Dec 24, 2018

Copy link
Copy Markdown
Owner

@aleksandrs-ledovskis Sorry, it fell through the cracks

@aleksandrs-ledovskis aleksandrs-ledovskis deleted the feature/skip-warnings-for-pluralization-branch-outs branch December 24, 2018 20:27
@aleksandrs-ledovskis

aleksandrs-ledovskis commented Mar 29, 2019

Copy link
Copy Markdown
Contributor Author

@glebm Could you cut a new release with changes done since v0.9.28 ?

@glebm glebm added this to the v0.9.29 milestone Mar 31, 2019
@glebm

glebm commented Mar 31, 2019

Copy link
Copy Markdown
Owner

@aleksandrs-ledovskis Released v0.9.29!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glebm glebm glebm left review comments

+1 more reviewer

@Gargron Gargron Gargron left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.9.29

Development

Successfully merging this pull request may close these issues.

3 participants

@aleksandrs-ledovskis @glebm @Gargron