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

Add support for keyword arguments for lanes in Ruby 3 #21587

Merged
merged 8 commits into from Oct 19, 2023

Conversation

AliSoftware
Copy link
Contributor

@AliSoftware AliSoftware commented Oct 19, 2023

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.
  • I've added or updated relevant unit tests.

Motivation and Context

In Ruby 2, when a method or a block expects keyword parameters, calling that method with a Hash (whose keys matches those keyword parameters) was valid and resulted in Ruby 2.x making an implicit conversion. In Ruby 3, that conversion has to be explicit (see this official article).

As a result, while writing lanes using keyword parameter syntax like below works in Ruby 2, it stopped working when running Fastlane in Ruby 3:

lane :kwparams do |name:, version:, interactive: true|
  UI.message("name: #{name}")
  UI.message("version: #{version}")
  UI.message("interactive: #{interactive}")
end

# Which is way more explicit, readable, and less verbose than the more common form
# that most people use when they don't know the keyword syntax exists for blocks
lane :hashparams do |options|
  UI.message("name: #{options[:name]}")
  UI.message("version: #{options[:version]}")
  UI.message("interactive: #{options[:interactive] || true}")
end

Description

This PR allows back the usage of keyword syntax in lanes block' parameters in Ruby 3, by detecting if the block provided to the lane expects at least one keyword (as opposed to just expecting a single positional parameter expected to be a Hash), and if so use the double-splat operator **options to make an explicit conversion of the options Hash (that our internal implementation passes to lane.call) into a keyword list before calling the lane's provided block.

Note: because the code actively checks if the block expects keywords or not, this change is not breaking, but just additive (i.e. the lane :foo do |options| syntax expecting a Hash will still work too)

Benefits of using the (sadly lesser-known) keyword syntax in your lanes

Note that this feature of using keyword parameters in lane blocks has always been working in Ruby 2, even though not many fastlane users might have known about it. As such, this PR simply makes it so this already-existing support for it in Ruby 2 doesn't break in Ruby 3.

Some benefits of using the keyword syntax for blocks in general (and for fastlane lane parameters in particular):

  • It's self-documenting the parameters that the lane accepts
  • Ruby will detect and error if you pass the wrong/unexpected parameter name. For example, kwparams(name: 'oops', version_name: 'bad') will make Ruby complain about version_name being an unexpected parameter for that kwparams lane (indeed, it should be version:, not version_name:).
  • Likewise, if you forget to pass a required parameter (one that doesn't have a default value), Ruby will also complain
  • Speaking of: this provides built-in support for default values of parameters of your lanes (e.g. see interactive: true in example above)

While on the contrary, if you use Hash parameters (e.g. do |options| …), you'll get none of those benefits, and as a result:

  • It's generally harder to see at a glance what are the options that the lane is supposed to accept (you have to read the code to know, or make sure your lane documentation is always up-to-date)
  • Default values have to be handled at the lane's implementation level (options[:interactive] || true)
  • If you accidentally pass incorrect options or make a typo in one of the options passed to your lane, it will likely result in obscure or easy-to-miss issues (with e.g. options[:artifacts] silently returning nil without any warning or error if you accidentally used artefacts instead of artifacts at call site)

Testing Steps

Check that CI passes on bundle exec rspec in both Ruby 2 and Ruby 3.

Alternatively, if you want to test this locally:

  • Checkout the first commit of this PR (1381e1a), which contains the unit tests/specs for that use case but not yet the fix.
  • Switch to using Ruby 2.x (e.g. rbenv local 2.7.4).
  • Run bundle exec rspec fastlane/spec/runner_spec.rb, and check there is no error.
  • Switch to using Ruby 3.x (e.g. rbenv local 3.2.2).
  • Run bundle exec rspec fastlane/spec/runner_spec.rb, and observe that the tests which are running the lane_kw_params lane of our test Fastfile are crashing with ArgumentError: missing keywords: :name, :version — because Ruby 3 doesn't do the implicit conversion of the Hash to keyword parameters so the method is called with one positional argument (the Hash) but no keyword argument.
  • Checkout the tip of this PR's branch, to now include the commit that contains the fix (6891acc)
  • Run bundle exec rspec fastlane/spec/runner_spec.rb again, and observe how the tests are now passing again.

While that previously worked implicitly in Ruby 2.x (with our internal implementation always passing a `Hash` in `lane.call(…)` but Ruby converting it as keywords dynamically if the block expected keywords), this has to be explicit in Ruby 3.x now.

See https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

context 'when a lane expects its parameters as keywords' do
def keywords_list(list)
# The way keyword lists appear in error messages is different in Windows & Linux vs macOS
Copy link
Member

Choose a reason for hiding this comment

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

🫨

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Just a minor comment to improve spec wording but overall this discovery is AMAZING, I love it! I think one of the benefits that I don't think you listed of using the keyword arguments is that the values are automatically "unwrapped", meaning you don't need to do e.g.

version = options[:version]

You simply just access version 😃

One thing that I missed in the specs was testing what happens if you pass nil to a keyword arguments, like:

it 'allows a required parameter to receive nil value' do
  result = @ff.runner.execute(:lane_kw_params, :ios, { version: "12.3", interactive: true, name: nil })
  expect(result).to eq('name: nil; version: "12.3"; interactive: true')
end

I haven't tested this myself but I believe it allows passing nil as a valid value, right? Not ideal IMO but I guess there's no much we can do about it with the current infra haha. Also not sure if it's possible to pass nil from fastlane's CLI 🤷‍♂️

Anyway, great contribution!! I'll for sure start using this the moment it gets shipped 😁

fastlane/spec/runner_spec.rb Outdated Show resolved Hide resolved
@AliSoftware
Copy link
Contributor Author

One thing that I missed in the specs was testing what happens if you pass nil to a keyword arguments, like:

Addressed in 944909f 🙂

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

💯 💯 💯

Can't wait to use this! 🤩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants