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

Included module method are overridden #23

Closed
cbeer opened this issue Jan 12, 2021 · 4 comments · Fixed by #24
Closed

Included module method are overridden #23

cbeer opened this issue Jan 12, 2021 · 4 comments · Fixed by #24

Comments

@cbeer
Copy link

cbeer commented Jan 12, 2021

In ostruct 0.3.2, we're seeing some surprising behavior for subclasses that include methods from other modules. Unlike ostruct 0.2, the included methods are now overridden with the ostruct accessors:

require 'ostruct'

module Bar
  def baz
    'from bar'
  end
end

class Foo < OpenStruct
  include Bar
end

Foo.new(baz: 'a').baz

# in ostruct 0.2.0:
# => 'from bar'

# in ostruct 0.3.2:
# => 'a'

But if the method is defined on the class itself, the method is not overridden:

class Quux < OpenStruct
  def baz
    'from quux'
  end
end

Quux.new(baz: 'a').baz

# in ostruct 0.2.0:
# => 'from quux'

# in ostruct 0.3.2:
# => 'from quux'

We're looking into some possible work-arounds for our usage, but I wanted to confirm that new behavior is working as expected. Thanks.

@marcandre
Copy link
Member

Thanks for opening this issue.

Indeed, the way I wrote the check to see where a method is defined compared to OpenStruct is incorrect. If you modify your example above with Bar being a class instead of a module it works as you expect it.

I will fix this shortly.

@cbeer
Copy link
Author

cbeer commented Jan 13, 2021

Thanks for the quick fix!

@marcandre
Copy link
Member

marcandre commented Jan 13, 2021

You're welcome. I'm waiting on the main CI and will then make a release

matzbot pushed a commit to ruby/ruby that referenced this issue Jan 13, 2021
@marcandre
Copy link
Member

Released in v0.3.3

mishina2228 added a commit to mishina2228/twurl that referenced this issue Apr 14, 2022
The following test cases fail when running CI in Ruby 3.0.

```
  1) Failure:
Twurl::CLI::OptionParsingTest#test_setting_host_updates_to_requested_value [/home/runner/work/twurl/twurl/test/cli_test.rb:214]:
Expected: "localhost:3000"
  Actual: "api.twitter.com"

  2) Failure:
Twurl::CLI::OptionParsingTest#test_setting_proxy_updates_to_requested_value [/home/runner/work/twurl/twurl/test/cli_test.rb:237]:
Expected: "localhost:80"
  Actual: nil

  3) Failure:
Twurl::CLI::OptionParsingTest#test_passing_no_ssl_option_disables_ssl [/home/runner/work/twurl/twurl/test/cli_test.rb:196]:
Expected false to be truthy.

  4) Failure:
Twurl::CLI::OptionParsingTest#test_specifying_a_request_method_extracts_and_normalizes_request_method [/home/runner/work/twurl/twurl/test/cli_test.rb:54]:
Expected: "put"
  Actual: "get"

  5) Failure:
Twurl::CLI::OptionParsingTest#test_protocol_is_stripped_from_host [/home/runner/work/twurl/twurl/test/cli_test.rb:221]:
Expected: "localhost:3000"
  Actual: "api.twitter.com"

  6) Failure:
Twurl::CLI::OptionParsingTest#test_passing_data_and_an_explicit_request_method_uses_the_specified_method [/home/runner/work/twurl/twurl/test/cli_test.rb:148]:
Expected: "delete"
  Actual: "post"

  7) Failure:
Twurl::Options::Test#test_ssl_is_enabled_if_the_protocol_is_https [/home/runner/work/twurl/twurl/test/cli_options_test.rb:18]:
Expected false to be truthy.

  8) Failure:
Twurl::Options::Test#test_base_url_is_built_from_protocol_and_host [/home/runner/work/twurl/twurl/test/cli_options_test.rb:13]:
Expected: "http://api.twitter.com"
  Actual: "https://api.twitter.com"
```

According to twitter#159 (comment),
the cause appears to be a bug in OpenStruct.
The bug was fixed in ruby/ostruct#23, and released as v0.3.3.
smaeda-ks pushed a commit to twitter/twurl that referenced this issue Apr 15, 2022
* Add GitHub Actions Workflow

* Remove Coveralls as it is not being used

#161 (comment)

* Address changes in RR v3.0.0

Now we need to take the block as a proc, not as an argument.
refs: rr/rr@d6da209#diff-a20b4cc1aac26c32a40206ad0776d4ff528201e97b737b7b3f0be1eb0e12e93dL44-L49

* Require RR v3

* Require OpenStruct v0.3.3+

The following test cases fail when running CI in Ruby 3.0.

```
  1) Failure:
Twurl::CLI::OptionParsingTest#test_setting_host_updates_to_requested_value [/home/runner/work/twurl/twurl/test/cli_test.rb:214]:
Expected: "localhost:3000"
  Actual: "api.twitter.com"

  2) Failure:
Twurl::CLI::OptionParsingTest#test_setting_proxy_updates_to_requested_value [/home/runner/work/twurl/twurl/test/cli_test.rb:237]:
Expected: "localhost:80"
  Actual: nil

  3) Failure:
Twurl::CLI::OptionParsingTest#test_passing_no_ssl_option_disables_ssl [/home/runner/work/twurl/twurl/test/cli_test.rb:196]:
Expected false to be truthy.

  4) Failure:
Twurl::CLI::OptionParsingTest#test_specifying_a_request_method_extracts_and_normalizes_request_method [/home/runner/work/twurl/twurl/test/cli_test.rb:54]:
Expected: "put"
  Actual: "get"

  5) Failure:
Twurl::CLI::OptionParsingTest#test_protocol_is_stripped_from_host [/home/runner/work/twurl/twurl/test/cli_test.rb:221]:
Expected: "localhost:3000"
  Actual: "api.twitter.com"

  6) Failure:
Twurl::CLI::OptionParsingTest#test_passing_data_and_an_explicit_request_method_uses_the_specified_method [/home/runner/work/twurl/twurl/test/cli_test.rb:148]:
Expected: "delete"
  Actual: "post"

  7) Failure:
Twurl::Options::Test#test_ssl_is_enabled_if_the_protocol_is_https [/home/runner/work/twurl/twurl/test/cli_options_test.rb:18]:
Expected false to be truthy.

  8) Failure:
Twurl::Options::Test#test_base_url_is_built_from_protocol_and_host [/home/runner/work/twurl/twurl/test/cli_options_test.rb:13]:
Expected: "http://api.twitter.com"
  Actual: "https://api.twitter.com"
```

According to #159 (comment),
the cause appears to be a bug in OpenStruct.
The bug was fixed in ruby/ostruct#23, and released as v0.3.3.

* Drop support for Ruby 2.4

- OpenStruct v0.3.3+ requires Ruby 2.5+.
- It's already reached EOL on 2020-03-31.

* Migrate CI from Travis CI to GitHub Actions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants