Remove usage of OpenStruct
#51510
Remove usage of OpenStruct
#51510
Conversation
rails-bot
bot
added
actionmailer
actionpack
actionview
activejob
activerecord
railties
labels
fatkodima
force-pushed
the
remove-ostruct
branch
from
c50896e
to
8748aaa
Compare
| @@ -355,9 +355,6 @@ Performance/DeletePrefix: | |||
| Performance/DeleteSuffix: | |||
| Enabled: true | |||
|
|
|||
| Performance/OpenStruct: | |||
There was a problem hiding this comment.
Shouldn't that cop have prevented use of OpenStruct?
I also see there is a Style/OpenStructUse cop now? If we're to remove it from the test suite might as well ensure it won't be re-introduced.
There was a problem hiding this comment.
All Performance cops are disabled for the test suite here. It would need to be explicitly enabled by path
| @@ -55,7 +55,7 @@ class ActiveRecord::Encryption::ConfigurableTest < ActiveRecord::EncryptionTestC | |||
| end | |||
|
|
|||
| test "installing autofiltered parameters will work with unnamed classes" do | |||
| application = OpenStruct.new(config: OpenStruct.new(filter_parameters: [])) | |||
| application = ActiveSupport::OrderedOptions[config: ActiveSupport::OrderedOptions[filter_parameters: []]] | |||
There was a problem hiding this comment.
I'm not too sure how I feel about using OrderedOption as a stand-in for OpenStruct.
From a purely pragmatic point of view, it's definitely superior as at least it's a class we control and we know it's loaded.
But I feel in many cases here, and espeically on the code just above, it may make more sense to use a Minitest::Mock as it is closer to the intent. WDYT?
There was a problem hiding this comment.
Turned out, that OpenStruct is not equivalent to ActiveSupport::OrderedOptions, as failing CI shows. The problem is described in #51469. ActiveSupport::OrderedOptions#respond_to_missing? returns true for all the calls and so the line
Just tried to convert to use mocks, and turned out that methods like is_a? should be mocked too. Meh. So probably just require "ostruct" in all the places?
There was a problem hiding this comment.
So probably just require "ostruct" in all the places?
Meh, I think the issue will just crop back up.
I'm tempted to suggest using Struct, it's a tiny bit more verbose but would do the job fine.
There was a problem hiding this comment.
@byroot Updated to use Structs. Please take a look.
`ostruct` was being implictly required by the `json` gem. But once it was upgraded, these tests failed to initialize `OpenStruct`. While we're trying to remove `ostruct` usage in #51510, CI is currently failing so I'm pushing these in the mean time.
|
Hey, I saw you're trying to remove these but CI was failing due to json bump, so I added more requires you'll need to get rid of in 50515fb |
Similar to 50515fb, make sure we require `ostruct` where we use `OpenStruct`, to get the build back to green, while we work to remove its usage on tests. (see #51510.) Sample error: ``` Error: ActiveRecord::Encryption::ConfigurableTest#test_installing_autofiltered_parameters_will_add_the_encrypted_attribute_as_a_filter_parameter_using_the_dot_notation: NameError: uninitialized constant ActiveRecord::Encryption::ConfigurableTest::OpenStruct test/cases/encryption/configurable_test.rb:45:in `block in <class:ConfigurableTest>' ```
fatkodima
force-pushed
the
remove-ostruct
branch
from
8748aaa
to
77c57b2
Compare
fatkodima
force-pushed
the
remove-ostruct
branch
from
77c57b2
to
041de49
Compare
|
Thank you! ❤️ |
Similar to 50515fb, make sure we require `ostruct` where we use `OpenStruct`, to get the build back to green, while we work to remove its usage on tests. (see rails#51510.) Sample error: ``` Error: ActiveRecord::Encryption::ConfigurableTest#test_installing_autofiltered_parameters_will_add_the_encrypted_attribute_as_a_filter_parameter_using_the_dot_notation: NameError: uninitialized constant ActiveRecord::Encryption::ConfigurableTest::OpenStruct test/cases/encryption/configurable_test.rb:45:in `block in <class:ConfigurableTest>' ```
Remove usage of `OpenStruct`
Follow up to #51491 (comment).
There was one more error regarding the missing
require "ostruct"after that PR merge (https://buildkite.com/rails/rails/builds/106158#018eaee1-7e2b-4e8a-9876-8d3c358d5576), so maybe we can just remove its usage instead?cc @byroot