8000 Added Ruby 3 fix for unique_utils by simonhildebrandt · Pull Request #532 · ffaker/ffaker · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Added Ruby 3 fix for unique_utils #532

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

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Added Ruby 3 fix for unique_utils #532

merged 2 commits into from
Aug 24, 2023

Conversation

simonhildebrandt
Copy link
Contributor
@simonhildebrandt simonhildebrandt commented Aug 23, 2023

During our Ruby 3 upgrade I noticed an argument error while doing this:

FFaker::Number.unique.number(digits: 1)

... turns out FFaker::UniqueUtils#method_missing doesn't handle keyword args the Ruby 3 way. This change (with test) fixes that.

@AlexWayfer AlexWayfer self-requested a review August 23, 2023 12:30
Copy link
Contributor
@AlexWayfer AlexWayfer left a comment

Choose a reason for hiding this comment

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

I agree with proposed changes, but it breaks Ruby 2.5 and 2.6.

I suggest to drop their support: they're not supported officially even for security.

The second way is to use ruby2_keywords: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/#a-compatible-delegation
But I don't like that much.

@marocchino
Copy link
Member

I just drop under ruby 2.7.

@AlexWayfer
Copy link
8000 Contributor

@simonhildebrandt rebase to the updated main please.

@marocchino
Copy link
Member

It would be very helpful if you also update the Changelog.md.

Copy link
Member
@marocchino marocchino left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks.

@AlexWayfer AlexWayfer merged commit 4931969 into ffaker:main Aug 24, 2023
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.

3 participants
0