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

Address edge case with table config param #339

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

krasnoukhov
Copy link
Contributor

@krasnoukhov krasnoukhov commented Jun 1, 2023

There's a weird edge case when config parameter is named table but at the same time config is not provided. I expect it could be a common thing, like a database table that is not provided by default. Added the specs for this scenario that fail before the patch.

On Ruby 3.0+ it fails with this:

Failures:

  1) Config::Options when Settings file is using keywords reserved for OpenStruct when empty should allow to access them via object member notation
     Failure/Error: expect(config.table).to be_nil
     
       expected: nil
            got: {}
     # ./spec/options_spec.rb:51:in `block (4 levels) in <main>'

  2) Config::Options when Settings file is using keywords reserved for OpenStruct when empty should allow to access them using [] operator
     Failure/Error: expect(config['table']).to be_nil
     
       expected: nil
            got: {}
     # ./spec/options_spec.rb:56:in `block (4 levels) in <main>'

Finished in 0.15976 seconds (files took 0.40783 seconds to load)
137 examples, 2 failures

Failed examples:

rspec ./spec/options_spec.rb:49 # Config::Options when Settings file is using keywords reserved for OpenStruct when empty should allow to access them via object member notation
rspec ./spec/options_spec.rb:54 # Config::Options when Settings file is using keywords reserved for OpenStruct when empty should allow to access them using [] operator

On Ruby 2.7:

Failures:

  1) Config::Options when Settings file is using keywords reserved for OpenStruct should allow to access them using [] operator
     Failure/Error: expect(config['table']).to eq('strawberry')
     
       expected: "strawberry"
            got: {:collect=>"banana", :count=>"lemon", :exit!=>"taro", :max=>"kumquat", :min=>"fig", :select=>"apple", :table=>"strawberry", :zip=>"cherry"}
     
       (compared using ==)
     
       Diff:
       @@ -1,8 +1,15 @@
       -"strawberry"
       +:collect => "banana",
       +:count => "lemon",
       +:exit! => "taro",
       +:max => "kumquat",
       +:min => "fig",
       +:select => "apple",
       +:table => "strawberry",
       +:zip => "cherry",
       
     # ./spec/options_spec.rb:32:in `block (3 levels) in <main>'

  2) Config::Options when Settings file is using keywords reserved for OpenStruct when empty should allow to access them using [] operator
     Failure/Error: expect(config['table']).to be_nil
     
       expected: nil
            got: {}
     # ./spec/options_spec.rb:56:in `block (4 levels) in <main>'

Finished in 0.13898 seconds (files took 0.362 seconds to load)
137 examples, 2 failures

Failed examples:

rspec ./spec/options_spec.rb:24 # Config::Options when Settings file is using keywords reserved for OpenStruct should allow to access them using [] operator
rspec ./spec/options_spec.rb:54 # Config::Options when Settings file is using keywords reserved for OpenStruct when empty should allow to access them using 

@pkuczynski pkuczynski requested a review from cjlarose June 1, 2023 20:48
@pkuczynski
Copy link
Member

@cjlarose what do you think? For me it looks legit...

@cjlarose
Copy link
Member

Yeah, found a relevant SO post about this issue with OpenStruct

Copy link
Member

@cjlarose cjlarose left a comment

Choose a reason for hiding this comment

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

Awesome work! Thanks!

@cjlarose cjlarose merged commit d8d1319 into rubyconfig:master Jun 15, 2023
@cjlarose
Copy link
Member

Published as version 4.2.1

@krasnoukhov
Copy link
Contributor Author

Thanks folks! Appreciate it :shipit:

danilogco added a commit to dcotecnologia/confset that referenced this pull request Oct 13, 2023
@pkuczynski pkuczynski added this to the 4.2.1 milestone Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants