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

Fix index for Polish books #422

Merged
merged 7 commits into from
Nov 9, 2021
Merged

Fix index for Polish books #422

merged 7 commits into from
Nov 9, 2021

Conversation

wilkus27
Copy link
Contributor

@wilkus27 wilkus27 commented Oct 13, 2021

Issue: #442

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #422 (8d3f4ab) into main (2d82d5a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #422   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         179      179           
  Lines        2710     2712    +2     
=======================================
+ Hits         2709     2711    +2     
  Misses          1        1           
Impacted Files Coverage Δ
lib/kitchen/directions/bake_index/v1.rb 100.00% <100.00%> (ø)
lib/kitchen/patches/i18n.rb 100.00% <100.00%> (ø)

@wilkus27 wilkus27 self-assigned this Oct 13, 2021
@wilkus27
Copy link
Contributor Author

@KarinaMendez2 Can you give me examples of Spanish words that start with special characters so I can test them? As I remember you use transliterate method for Spanish books.

Copy link
Contributor

@kswanson33 kswanson33 left a comment

Choose a reason for hiding this comment

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

I like what you've done here! One naming-related comment, but otherwise it looks great to me 🎉

@@ -4,6 +4,10 @@

# rubocop:disable Style/Documentation
module I18n
def self.group_by(character)
Copy link
Contributor

Choose a reason for hiding this comment

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

group_by as a name makes sense in the context of indexes, but this function doesn't group anything, it just decides what character to return based on locale. Maybe a better name would be something like transliterate_for_index or character_to_group, or something I haven't thought of! lmk what you think :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good point 👍 I didn't think about it. I like very much second proposition. It's more descriptive. I made a fix for that.

Comment on lines +4 to +16
def stub_locales(hash, locale: nil)
@locales_are_stubbed = true

language = locale || :test

I18n.config.available_locales = %i[test en es pl]
allow_any_instance_of(I18n::Config).to receive(:backend).and_return(
I18n::Backend::Simple.new.tap do |backend|
backend.store_translations 'test', hash
backend.store_translations language, hash
end
)

assign_i18n_dot_locale(:test)
assign_i18n_dot_locale(language)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@wilkus27 wilkus27 merged commit 3f72d38 into main Nov 9, 2021
@wilkus27 wilkus27 deleted the fix-index-pl-psychology branch November 9, 2021 15:36
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.

3 participants