Skip to content

Commit

Permalink
Clean up conditionals based on prior Ruby versions
Browse files Browse the repository at this point in the history
Used standardrb in the process, so update the minimum Ruby version to
3.0 for this to work.
  • Loading branch information
mike-burns committed Dec 29, 2023
1 parent 211c088 commit 36bd065
Show file tree
Hide file tree
Showing 7 changed files with 16 additions and 29 deletions.
2 changes: 1 addition & 1 deletion .standard.yml
@@ -1 +1 @@
ruby_version: "2.5"
ruby_version: "3.0"
22 changes: 5 additions & 17 deletions lib/factory_bot/decorator.rb
Expand Up @@ -6,24 +6,12 @@ def initialize(component)
@component = component
end

if ::Gem::Version.new(::RUBY_VERSION) >= ::Gem::Version.new("2.7")
class_eval(<<~RUBY, __FILE__, __LINE__ + 1)
def method_missing(...) # rubocop:disable Style/MethodMissingSuper, Style/MissingRespondToMissing
@component.send(...)
end
def send(...)
__send__(...)
end
RUBY
else
def method_missing(name, *args, &block) # rubocop:disable Style/MissingRespondToMissing
@component.send(name, *args, &block)
end
def method_missing(...) # rubocop:disable Style/MethodMissingSuper
@component.send(...)
end

def send(symbol, *args, &block)
__send__(symbol, *args, &block)
end
def send(...)
__send__(...)
end

def respond_to_missing?(name, include_private = false)
Expand Down
4 changes: 2 additions & 2 deletions lib/factory_bot/definition_proxy.rb
Expand Up @@ -119,8 +119,8 @@ def method_missing(name, *args, &block) # rubocop:disable Style/MissingRespondTo
# end
#
# Except that no globally available sequence will be defined.
def sequence(name, *args, &block)
sequence = Sequence.new(name, *args, &block)
def sequence(name, ...)
sequence = Sequence.new(name, ...)
FactoryBot::Internal.register_inline_sequence(sequence)
add_attribute(name) { increment_sequence(sequence) }
end
Expand Down
7 changes: 3 additions & 4 deletions lib/factory_bot/evaluator.rb
Expand Up @@ -35,14 +35,13 @@ def association(factory_name, *traits_and_overrides)

attr_accessor :instance

def method_missing(method_name, *args, &block)
def method_missing(method_name, ...)
if @instance.respond_to?(method_name)
@instance.send(method_name, *args, &block)
@instance.send(method_name, ...)
else
SyntaxRunner.new.send(method_name, *args, &block)
SyntaxRunner.new.send(method_name, ...)
end
end
ruby2_keywords :method_missing if respond_to?(:ruby2_keywords, true)

def respond_to_missing?(method_name, _include_private = false)
@instance.respond_to?(method_name) || SyntaxRunner.new.respond_to?(method_name)
Expand Down
4 changes: 2 additions & 2 deletions lib/factory_bot/syntax/default.rb
Expand Up @@ -25,8 +25,8 @@ def factory(name, options = {}, &block)
end
end

def sequence(name, *args, &block)
Internal.register_sequence(Sequence.new(name, *args, &block))
def sequence(name, ...)
Internal.register_sequence(Sequence.new(name, ...))
end

def trait(name, &block)
Expand Down
2 changes: 1 addition & 1 deletion spec/acceptance/attributes_for_destructuring.rb
Expand Up @@ -15,7 +15,7 @@
it "supports being destructured" do
# rubocop:disable Lint/Syntax
attributes_for(:user) => {name:, **attributes}
# rubocop:disable Lint/Syntax
# rubocop:enable Lint/Syntax

expect(name).to eq("John Doe")
expect(attributes.keys).to eq([:email])
Expand Down
4 changes: 2 additions & 2 deletions spec/acceptance/attributes_for_spec.rb
@@ -1,5 +1,5 @@
if RUBY_VERSION >= "3.0" && RUBY_ENGINE != "truffleruby"
require_relative "./attributes_for_destructuring"
if RUBY_ENGINE != "truffleruby"
require_relative "attributes_for_destructuring"
end

describe "a generated attributes hash" do
Expand Down

5 comments on commit 36bd065

@pboling
Copy link
Contributor

@pboling pboling commented on 36bd065 Mar 6, 2024

Choose a reason for hiding this comment

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

Was this intentionally released as a non-breaking change, and thus a "not-a-major" version bump?

Using the ... operator, as noted in the git commit comment, is Ruby 3.0+ only. For Rails apps this isn't a huge deal, as they only deal with one version of Ruby at a time. For other libraries that depend on factory_bot this is awful (e.g. https://github.com/pboling/activerecord-transactionable/actions/runs/8169704061/job/22334293843). In that link the latest (broken) gem is installing fine on Ruby 2.5.

I know there is a fair bit of disagreement over how SemVer is interpreted... and this is not serving the community very well.

The person who invented SemVer has recently written an opinion on this topic that I agree with:
https://tom.preston-werner.com/2022/05/23/major-version-numbers-are-not-sacred.html

  1. Good - Major Version Bump
  2. Stinky, but tolerable - update the minimum ruby version of the gem, to 3.0 in this case, in a patch release (this did not happen!)
  3. To the Pain - just break the code, and drop it like it's hot (this is what happened 😿 )

I love the gem. Thanks for all you've done, but I hope this process can improve somehow!

@mike-burns
Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting points.

I think the real issue you have is this PR: #1612

In your proposal, any time we drop an EOL'ed Ruby, we'd bump the major?

@pboling
Copy link
Contributor

@pboling pboling commented on 36bd065 Mar 7, 2024

Choose a reason for hiding this comment

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

Yes, it was fixed already in that PR, and by 6.4.6 release. 6.4.5 is always going to be sitting there ready to break my projects that support older Rubies, and I think that is demonstrative of one reason why a "major version" mentality around dropping old Rubies is beneficial.

So, yes, I'd propose that as a policy, a Ruby version drop only happens with a major version bump.

Perhaps it would have downsides to bump major versions frequently, but if we take rake as an example of a project that did a lot of major bumping - it doesn't seem to have incurred much wrath. :)

I think it is useful to place high value on dropping major Ruby versions so that it is not a decision lightly made.

@mike-burns
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to figure out the situation, perhaps you can walk me through this.

I definitely understand not upgrading Ruby and not upgrading the libraries. That's what I do in my personal life. I understand keeping Ruby and the libraries up to date. That's what I do in my professional life.

But I don't understand keeping libraries up to date but using an EOL'ed Ruby. Why wouldn't you also upgrade Ruby?

@pboling
Copy link
Contributor

@pboling pboling commented on 36bd065 Mar 7, 2024

Choose a reason for hiding this comment

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

But I don't understand keeping libraries up to date but using an EOL'ed Ruby. Why wouldn't you also upgrade Ruby?

I am not suggesting that we not upgrade Ruby at all, each library should do that according to their needs. RSpec still supports Ruby 1.8 and is one of the most popular gems ever. I write a lot of RSpec plugin gems, and I aim for Ruby version parity with RSpec. When RSpec drops old Rubies, I will for my RSpec plugins as well. Upgrading Ruby isn't really relevant here.

The issue is dropping support for old EOL'd Rubies (which I love to see!) without a major version bump (which I don't like to see).

I'm merely suggesting that it be policy going forward that dropping a minor version of Ruby be equated with a major version bump of the gem.

OK, but why, specifically?

The official recommendation of rubygems, bundler, and SemVer-following projects, is that dependencies be specified as: ~> 1.0, which means that patch updates, and minor udpates, will be pulled in. If a patch/minor update drops an EOL'd Ruby it will break my code unless I also drop the same Ruby support (not always immediately possible), or pin the version. On a large project there can be many blockers to upgrading Ruby.

Please sign in to comment.