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

Support Ruby 3.2 #81

Merged
merged 2 commits into from
Dec 28, 2022
Merged

Support Ruby 3.2 #81

merged 2 commits into from
Dec 28, 2022

Conversation

sue445
Copy link
Collaborator

@sue445 sue445 commented Nov 24, 2022

ref #80

Refinement#include is removed since ruby 3.2
https://bugs.ruby-lang.org/issues/17429

So I use Refinement#import_methods on ruby 3.2+

@joker1007 This patch actually works, but it's a little dirty...
What do you think?

Close #80

@sue445 sue445 changed the title Support Ruby3.2 Support Ruby 3.2 Nov 24, 2022
module RSpec
module Parameterized
module TableSyntaxImplement
extend BindingNinja
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[NOTE] This doesn't work on ruby 3.2 😫

$ ruby -v
ruby 3.2.0preview3 (2022-11-24) [x86_64-darwin21]

$ bundle exec rspec
warning: parser/current is loading parser/ruby32, which recognizes3.2.0-dev-compliant syntax, but you are running 3.2.0.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
/Users/sue445/workspace/github.com/tomykaira/rspec-parameterized/lib/rspec/parameterized/table_syntax.rb:30: warning: RSpec::Parameterized::TableSyntaxImplement has ancestors, but Refinement#import_methods doesn't import their methods
/Users/sue445/workspace/github.com/tomykaira/rspec-parameterized/lib/rspec/parameterized/table_syntax.rb:34: warning: RSpec::Parameterized::TableSyntaxImplement has ancestors, but Refinement#import_methods doesn't import their methods
/Users/sue445/workspace/github.com/tomykaira/rspec-parameterized/lib/rspec/parameterized/table_syntax.rb:38: warning: RSpec::Parameterized::TableSyntaxImplement has ancestors, but Refinement#import_methods doesn't import their methods
/Users/sue445/workspace/github.com/tomykaira/rspec-parameterized/lib/rspec/parameterized/table_syntax.rb:42: warning: RSpec::Parameterized::TableSyntaxImplement has ancestors, but Refinement#import_methods doesn't import their methods
/Users/sue445/workspace/github.com/tomykaira/rspec-parameterized/lib/rspec/parameterized/table_syntax.rb:46: warning: RSpec::Parameterized::TableSyntaxImplement has ancestors, but Refinement#import_methods doesn't import their methods
/Users/sue445/workspace/github.com/tomykaira/rspec-parameterized/lib/rspec/parameterized/table_syntax.rb:50: warning: RSpec::Parameterized::TableSyntaxImplement has ancestors, but Refinement#import_methods doesn't import their methods

An error occurred while loading ./spec/parametarized_spec.rb.
Failure/Error: "integers"      | 1                     | 2                     | 3

NoMethodError:
  undefined method `|' for "integers":String
# ./spec/parametarized_spec.rb:174:in `block (4 levels) in <top (required)>'
# ./lib/rspec/parameterized.rb:107:in `instance_eval'
# ./lib/rspec/parameterized.rb:107:in `define_cases'
# ./lib/rspec/parameterized.rb:86:in `with_them'
# ./spec/parametarized_spec.rb:180:in `block (3 levels) in <top (required)>'
# ./spec/parametarized_spec.rb:170:in `block (2 levels) in <top (required)>'
# ./spec/parametarized_spec.rb:134:in `block in <top (required)>'
# ./spec/parametarized_spec.rb:13:in `<top (required)>'
Run options: include {:current=>true}

All examples were filtered out; ignoring {:current=>true}


module RSpec
module Parameterized
module TableSyntaxImplement
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sue445 sue445 requested a review from joker1007 November 24, 2022 15:27
@sue445 sue445 marked this pull request as ready for review November 24, 2022 15:28
@sue445
Copy link
Collaborator Author

sue445 commented Dec 8, 2022

@joker1007 ping

@@ -16,6 +16,7 @@ I was inspired by [udzura's mock](https://gist.github.com/1881139).}
gem.add_dependency('unparser')
gem.add_dependency('proc_to_ast')
gem.add_dependency('binding_ninja', '>= 0.2.3')
gem.add_dependency('binding_of_caller')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can remove both binding_ninja and binding_of_caller from the dependency.
I think it would be better to trigger a LoadError at require time and notice a warning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doing that might make your gemspec cleaner.

However instead, gem users need to write the following in Gemfile, which is inconvenient.

gem "rspec-parameterized"

# for Ruby 3.2+
gem "binding_of_caller"

# for Ruby < 3.2
gem "binding_ninja"

They and we will need to manually modify the Gemfile when upgrading.

So I would rather remove the dependency on binding_ninja and use only binding_of_caller instead.

This keeps the gem users's upgrade costs low and keeps the implementation of this gem simple.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I also think it is inconvenient.
But TableSyntax is an optional feature. and binding_ninja and binding_of_caller are native extension libraries that are unnecessary dependencies for some people.

I think requiring all users to compile them makes it harder for casual users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So How do you think about carving out TableSyntax into a other gem? (e.g. rspec-parameterized-table_syntax )

Example. 1

rspec-parameterized.gemspec

gem.add_dependency 'rspec'

# TODO: Write other minimum required dependencies.

rspec-parameterized-table_syntax.gemspec

gem.add_dependency 'rspec-parameterized'
gem.add_dependency 'binding_of_caller'

Example. 2

This is similar to rails and rspec gems

rspec-parameterized-core.gemspec

gem.add_dependency 'rspec'

# TODO: Write other minimum required dependencies.

rspec-parameterized-table_syntax.gemspec

gem.add_dependency 'rspec-parameterized-core'
gem.add_dependency 'binding_of_caller'

rspec-parameterized.gemspec

gem.add_dependency 'rspec-parameterized-core'
gem.add_dependency 'rspec-parameterized-table_syntax'

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!
It's a better solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Separating the gems is a good idea, but separating the gems is not mandatory for Ruby 3.2 support. Also, I think Ruby 3.2 users will be inconvenienced until the gems are separated.

Therefore, we do Ruby 3.2 support with minimal changes in this major version and separate the gems in the next major version? (#82)

I have written a patch at 0760ea6 to minimize dependencies for Ruby 3.2 support

@joker1007
Copy link
Collaborator

@sue445
I'm sorry for the late replaying. 🙇
I've submitted one comment.

Thank you!

@sue445 sue445 force-pushed the ruby3.2 branch 2 times, most recently from 0f1427a to bc0054c Compare December 24, 2022 17:44
@sue445
Copy link
Collaborator Author

sue445 commented Dec 28, 2022

I recently spoke directly with @joker1007 and I have reached an agreement on this patch.

@sue445 sue445 merged commit f49f886 into master Dec 28, 2022
@sue445 sue445 deleted the ruby3.2 branch December 28, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

table_syntax.rb Refinement#include is deprecated and will be removed in Ruby 3.2
2 participants