-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
change i18n directory check #899
Conversation
suggestions added Reviewed 1 of 1 files at r1. lib/react_on_rails/locales_to_js.rb, line 8 at r1 (raw file):
what if i18n_dir is nil? I think we need to check both cases... We should also have an added unit test for this. Please see https://github.com/shakacode/react_on_rails/blob/master/CONTRIBUTING.md for details like adding a change log entry, etc. Comments from Reviewable |
@hakongit Any update? |
Please see comments Reviewed 3 of 3 files at r2. CHANGELOG.md, line 14 at r2 (raw file):
Please follow formatting examples for other changes. lib/react_on_rails/locales_to_js.rb, line 9 at r2 (raw file):
Did you test the case of setting this to nil? Please look at code here: https://github.com/shakacode/react_on_rails/blob/master/lib/react_on_rails/configuration.rb#L21 Not sure if we'll hit all these cases. spec/react_on_rails/locales_to_js_spec.rb, line 21 at r2 (raw file):
all these blank lines won't pass linter Please clarify the different test cases, preferably in the test descriptions. Incidentally, the default is "", not nil, but both should mean the same thing. Additionally, Ruby for Directory present may have changed in 2.4! so this change needs verification on both 2.4 and some prior Ruby. See Comments from Reviewable |
…hitespaces, updated changelog
@hakongit I got a chance to look into this. I'm guessing that you're setting the value to a real directory or file in the initializer rather than a string, so
while
We need to address:
Please keep in mind that the check in locales_to_js.rb is supposed to simply return if there is no directory specified. The error condition is having a non-blank string that points to a non-existent directory (or a file). Maybe we should handle the case of a File or Dir object? Reviewed 3 of 3 files at r3. lib/react_on_rails/locales_to_js.rb, line 8 at r3 (raw file):
I'd rather have: if !i18n_dir.nil? && File.directory?(i18n_dir) lib/react_on_rails/locales_to_js.rb, line 9 at r3 (raw file):
grammar and spelling:
Comments from Reviewable |
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed. spec/react_on_rails/locales_to_js_spec.rb, line 16 at r3 (raw file):
The logic of these is the opposite of the current behavior. We're supposed to short circuit this if a nil or blank string is used. Comments from Reviewable |
@hakongit Please correct the CHANGELOG.md comment and I'll merge. Thank you! Reviewed 2 of 2 files at r4. Comments from Reviewable |
updated CHANGELOG.md, is the language/format good?
2017-08-11 1:57 GMT+02:00 Justin Gordon <[email protected]>:
… @hakongit <https://github.com/hakongit> Please correct the CHANGELOG.md
comment and I'll merge. Thank you!
------------------------------
Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 7 unresolved
discussions, some commit checks failed.
------------------------------
*Comments from Reviewable
<https://reviewable.io:443/reviews/shakacode/react_on_rails/899#-:-KrD_5x5QRbOlF7g4gq1:b-40mpj2>*
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#899 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAuGoxlz5NQ_Fp37KV7TKdMAj6lbIeasks5sW5jngaJpZM4ObROG>
.
|
rebase on top of master and see my one comment Review status: 2 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed. CHANGELOG.md, line 14 at r5 (raw file):
See that all other entries indicate the PR and the author. Comments from Reviewable |
@hakongit Please be sure to check CI after you submit:
|
Ok it fails if i18n_yml_dir is nil, would it behave as wanted if we add
"return unless !i18n_yml_dir.nil?" in the initalize function?
2017-08-20 8:26 GMT+02:00 Justin Gordon <[email protected]>:
… @hakongit <https://github.com/hakongit> Please be sure to check CI after
you submit:
Failures:
1) ReactOnRails::TestHelper::EnsureAssetsCompiled#ensureAssetsCompiled when assets are not up to date compiles the webpack assets
Failure/Error:
locale_files.each do |f|
translation = YAML.safe_load(File.open(f))
key = translation.keys[0]
val = flatten(translation[key])
translations = translations.deep_merge(key => val)
defaults = defaults.deep_merge(flatten_defaults(val)) if key == default_locale
end
NoMethodError:
undefined method `each' for nil:NilClass
# ./lib/react_on_rails/locales_to_js.rb:87:in `generate_translations'
# ./lib/react_on_rails/locales_to_js.rb:12:in `initialize'
# ./lib/react_on_rails/test_helper/ensure_assets_compiled.rb:32:in `new'
# ./lib/react_on_rails/test_helper/ensure_assets_compiled.rb:32:in `call'
# ./lib/react_on_rails/test_helper.rb:84:in `ensure_assets_compiled'
# ./spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb:32:in `invoke_ensurer_with_doubles'
# ./spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb:18:in `block (4 levels) in <top (required)>'
# /home/travis/.rvm/gems/ruby-2.4.1/gems/rspec-retry-0.5.4/lib/rspec/retry.rb:112:in `block in run'
# /home/travis/.rvm/gems/ruby-2.4.1/gems/rspec-retry-0.5.4/lib/rspec/retry.rb:101:in `loop'
# /home/travis/.rvm/gems/ruby-2.4.1/gems/rspec-retry-0.5.4/lib/rspec/retry.rb:101:in `run'
# /home/travis/.rvm/gems/ruby-2.4.1/gems/rspec-retry-0.5.4/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
# /home/travis/.rvm/gems/ruby-2.4.1/gems/rspec-retry-0.5.4/lib/rspec/retry.rb:30:in `block (2 levels) in setup'
2) ReactOnRails::TestHelper::EnsureAssetsCompiled#ensureAssetsCompiled when assets are up to date does nothing
Failure/Error:
locale_files.each do |f|
translation = YAML.safe_load(File.open(f))
key = translation.keys[0]
val = flatten(translation[key])
translations = translations.deep_merge(key => val)
defaults = defaults.deep_merge(flatten_defaults(val)) if key == default_locale
end
NoMethodError:
undefined method `each' for nil:NilClass
# ./lib/react_on_rails/locales_to_js.rb:87:in `generate_translations'
# ./lib/react_on_rails/locales_to_js.rb:12:in `initialize'
# ./lib/react_on_rails/test_helper/ensure_assets_compiled.rb:32:in `new'
# ./lib/react_on_rails/test_helper/ensure_assets_compiled.rb:32:in `call'
# ./lib/react_on_rails/test_helper.rb:84:in `ensure_assets_compiled'
# ./spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb:32:in `invoke_ensurer_with_doubles'
# ./spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb:27:in `block (4 levels) in <top (required)>'
# /home/travis/.rvm/gems/ruby-2.4.1/gems/rspec-retry-0.5.4/lib/rspec/retry.rb:112:in `block in run'
# /home/travis/.rvm/gems/ruby-2.4.1/gems/rspec-retry-0.5.4/lib/rspec/retry.rb:101:in `loop'
# /home/travis/.rvm/gems/ruby-2.4.1/gems/rspec-retry-0.5.4/lib/rspec/retry.rb:101:in `run'
# /home/travis/.rvm/gems/ruby-2.4.1/gems/rspec-retry-0.5.4/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
# /home/travis/.rvm/gems/ruby-2.4.1/gems/rspec-retry-0.5.4/lib/rspec/retry.rb:30:in `block (2 levels) in setup'
Finished in 0.66301 seconds (files took 1.11 seconds to load)
169 examples, 2 failures
Failed examples:
rspec ./spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb:16 # ReactOnRails::TestHelper::EnsureAssetsCompiled#ensureAssetsCompiled when assets are not up to date compiles the webpack assets
rspec ./spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb:25 # ReactOnRails::TestHelper::EnsureAssetsCompiled#ensureAssetsCompiled when assets are up to date does nothing
rake aborted!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#899 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAuGo77FEH-BsaBYJnF-1ioa5-sUfDzAks5sZ9GNgaJpZM4ObROG>
.
|
Can you share the full block of code @hakongit? |
Thanks @hakongit Reviewed 1 of 1 files at r6, 1 of 1 files at r7. Comments from Reviewable |
I'll merge once I can get this pass CI. |
@hakongit I'd like to get this merged. Can you please rebase this on top of master? |
…hitespaces, updated changelog
@justin808 sure, I have done it now (?) |
@hakongit Please review your diffs. I recommend you squash all your commits into one nice tidy commit. https://github.com/shakacode/react_on_rails/pull/899/files |
done :-)
2017-10-25 2:49 GMT+02:00 Justin Gordon <[email protected]>:
… @hakongit <https://github.com/hakongit> Please review your diffs. I
recommend you squash all your commits into one nice tidy commit.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#899 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAuGo9uJxJUppvo26yxkJEd1eFX9XRYiks5svoW0gaJpZM4ObROG>
.
|
@hakongit Please confirm that if somebody had used "blank" ( Nevertheless, that's probably an easy fix, so I'm OK as-is, as the example configuration uses a directory and not a string. |
fixed and improved configuration checks broken by #899
i18n_dir.present? returned false even though directory existed, changing for directory seems to give intended result.
This change is