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

change i18n directory check #899

Merged
merged 32 commits into from
Oct 25, 2017
Merged

change i18n directory check #899

merged 32 commits into from
Oct 25, 2017

Conversation

hakongit
Copy link
Contributor

@hakongit hakongit commented Jul 18, 2017 •
edited by justin808
Loading

i18n_dir.present? returned false even though directory existed, changing for directory seems to give intended result.

This change is Reviewable

@justin808
Copy link
Member

suggestions added

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

lib/react_on_rails/locales_to_js.rb, line 8 at r1 (raw file):

  class LocalesToJs
    def initialize
      return unless i18n_dir.directory?

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

@justin808
Copy link
Member

@hakongit Any update?

@justin808
Copy link
Member

Please see comments

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

CHANGELOG.md, line 14 at r2 (raw file):

*Please add entries here for your pull requests.*
- Changes check in locales_to_js to check for i18n_dir.blank? and i18n_dir.directory?

Please follow formatting examples for other changes.

lib/react_on_rails/locales_to_js.rb, line 9 at r2 (raw file):

    def initialize
      if i18n_dir.blank?
        raise "i18n_dir missing, did you set i18n_dir in react_on_rails intializer?"

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):

    end

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

https://forum.shakacode.com/t/yak-of-the-week-ruby-2-4-pathname-empty-changed-to-look-at-file-size/901

Comments from Reviewable

@justin808
Copy link
Member

@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

some_empty_directory.present? == false

while

some_empty_directory_string.present? == true

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.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

lib/react_on_rails/locales_to_js.rb, line 8 at r3 (raw file):

  class LocalesToJs
    def initialize
      unless File.directory?(i18n_dir.to_s)

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):

    def initialize
      unless File.directory?(i18n_dir.to_s)
        raise "i18n_dir is not a directory did you set i18n_dir in react_on_rails intializer?"

grammar and spelling:

config.i18n_dir is set and it is not a directory. Did you set config.i18n_dir in the react_on_rails initializer?

Comments from Reviewable

@justin808
Copy link
Member

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):

        config.i18n_dir = ''
      end
      expect { ReactOnRails::LocalesToJs.new }.to raise_error(/did you set i18n_dir in react_on_rails intializer?/)

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

@justin808
Copy link
Member

@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

@hakongit
Copy link
Contributor Author

hakongit commented Aug 11, 2017 via email

@justin808
Copy link
Member

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):

*Please add entries here for your pull requests.*
- Fixes check for i18n_dir in LocalesToJs returning false when i18n_dir was set.

See that all other entries indicate the PR and the author.

Comments from Reviewable

@justin808
Copy link
Member

@hakongit
image

@justin808
Copy link
Member

@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!

@hakongit
Copy link
Contributor Author

hakongit commented Aug 27, 2017 via email

@justin808
Copy link
Member

Ok it fails if i18n_yml_dir is nil

Can you share the full block of code @hakongit?

@justin808
Copy link
Member

:lgtm:

Thanks @hakongit

Reviewed 1 of 1 files at r6, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

Comments from Reviewable

@justin808
Copy link
Member

I'll merge once I can get this pass CI.

@justin808
Copy link
Member

@hakongit I'd like to get this merged. Can you please rebase this on top of master?

@hakongit
Copy link
Contributor Author

hakongit commented Oct 24, 2017 •
edited
Loading

@justin808 sure, I have done it now (?)

@justin808
Copy link
Member

justin808 commented Oct 25, 2017 •
edited
Loading

@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

@hakongit
Copy link
Contributor Author

hakongit commented Oct 25, 2017 via email

@justin808
Copy link
Member

@hakongit Please confirm that if somebody had used "blank" ("") before and they upgrade to this fix, then they probably will be surprised.

Nevertheless, that's probably an easy fix, so I'm OK as-is, as the example configuration uses a directory and not a string.

@justin808 justin808 merged commit 820ac3c into shakacode:master Oct 25, 2017
This was referenced Oct 27, 2017
Merged
Merged
justin808 pushed a commit that referenced this pull request Oct 28, 2017
@justin808
fixing configuration checks (#967)
882cc5d 
fixed and improved configuration checks broken by #899
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
waiting for submitter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hakongit @justin808