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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Dec 24, 2018
Merged

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

merged 1 commit into from
Dec 24, 2018

Conversation

aleksandrs-ledovskis
Copy link
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
lib/i18n/tasks/data/tree/siblings.rb Outdated
@@ -10,6 +10,9 @@ module I18n::Tasks::Data::Tree
# in case of an empty parent (nil) it represents a forest
# 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
Copy link
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

Copy link
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
Copy link
Contributor Author

Build is green + rebased on latest master

glebm
glebm reviewed Nov 4, 2018
View reviewed changes
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?
Copy link
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)

Copy link
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
Copy link
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
Copy link
Owner

glebm commented Dec 24, 2018

@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
Copy link
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
Copy link
Owner

glebm commented Mar 31, 2019

@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

@Gargron Gargron Gargron left review comments

@glebm glebm glebm left review comments

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