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

Use shorthand lambda syntax #343

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Use shorthand lambda syntax #343

merged 1 commit into from
Jan 25, 2024

Conversation

robinjam
Copy link
Contributor

@robinjam robinjam commented Jan 25, 2024

Fixes #341

SimpleDelegator was intercepting the lambda call via method_missing, and delegating to Kernel#lambda, passing the given block. This explodes on Ruby 3.3 because lambda only accepts block literals now.

Replacing the lambda call with this shorthand syntax sidesteps the issue. RuboCop is unhappy because it prefers the old syntax for multiline lambdas, so I've had to disable that cop.

Tested locally against Content Data Admin.

Fixes #341

`SimpleDelegator` was intercepting the `lambda` call via `method_missing`, and delegating to `Kernel#lambda`, passing the given block. This explodes on Ruby 3.3 because `lambda` only accepts block literals now.

Replacing the `lambda` call with this shorthand syntax sidesteps the issue.
@robinjam robinjam marked this pull request as ready for review January 25, 2024 10:30
@robinjam robinjam requested a review from sengi January 25, 2024 10:42
Copy link
Contributor

@sengi sengi left a comment

Choose a reason for hiding this comment

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

Fix looks appropriate to me FWIW :) I'm sure you have a deeper knowledge of Ruby than I though!

@sengi
Copy link
Contributor

sengi commented Jan 25, 2024

I'm pretty suspicious of the use of SimpleDelegator here (in the first place) but I suspect pulling on that string might lead to a much bigger refactor — so I think your fix is the right one at the right time.

@robinjam
Copy link
Contributor Author

I'm pretty suspicious of the use of SimpleDelegator here but I suspect pulling on that string might lead to a much bigger refactor — so I think your fix is the right one at the right time.

Agreed, I'm always a little wary of SimpleDelegator and it certainly feels like a code smell here. Something to look into in the future

@robinjam robinjam merged commit 942ecdd into main Jan 25, 2024
11 checks passed
@robinjam robinjam deleted the use-lambda-shorthand branch January 25, 2024 11:01
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.

Fails on Ruby 3.3 with lambda method requires a literal block.
2 participants