-
Notifications
You must be signed in to change notification settings - Fork 22
Add support for yml filenames where locale is used as prefix #22
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,6 +102,22 @@ def test_create_missing_translations_in_top_level_file | |
assert_nil translation.text | ||
end | ||
|
||
def test_create_missing_keys_uses_replace_locale_in_path_transformation | ||
calls = [] | ||
@store.define_singleton_method(:replace_locale_in_path) do |*args| | ||
calls << args | ||
"/tmp/special_path/en.yml" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're just testing that you've stubbed a method. No go. Can't we just call the method, and assert the outputted path? 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, see the two tests at |
||
end | ||
|
||
@store.add_translation Translation.new(name: "da.app_name", text: "Oversætter", file: "/tmp/da.yml") | ||
@store.add_locale("en") | ||
@store.create_missing_keys | ||
|
||
assert_equal(1, calls.count) | ||
assert_equal(['da', 'en', '/tmp/da.yml'], calls.first) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't see the point in testing argument positions, sounds like a bag of hurt going forward 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am testing, that the interface defined by |
||
assert_equal "/tmp/special_path/en.yml", @store.translations["en.app_name"].file | ||
end | ||
|
||
def test_from_yaml | ||
input = { | ||
da: { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method extraction is fine, but what's the use case for making this public API? Would rather it was just a private method in
store.rb
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also think
path
comes more naturally as the first argument.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to use IYE not only through the browser frontend, but in migration scripts etc. too. So having the
nest_hash
andflatten_hash
methods available was pretty useful and the same applies to this new method in my opinion.Besides, why not make it public? It is documented and tested.