-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Drop support for EOL Ruby versions (2.5
and 2.6
)
#2538
Drop support for EOL Ruby versions (2.5
and 2.6
)
#2538
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thanks for working on this!
I left a couple of questions regarding the tests we probably still want to keep.
assert_raise Date::Error do | ||
::Date.new(@today.year - @min, @today.month, @today.day) | ||
end | ||
|
||
assert_raise Date::Error do | ||
::Date.new(@today.year - @max, @today.month, @today.day) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we want to remove the tests for version < 2.7 and we don't need the version checks anymore.
But we still want to keep the tests that check that the error raised is a Date::Error
, right? I don't think you need to remove them.
what do you think of also removing Ruby versions older than 2.7 from the test matrix? Here: faker/.github/workflows/ruby.yml Line 38 in 4ffc714
|
…ap_year and test_invalid_south_african_id_number
@thdaraujo I removed the ruby versions older than 2.7 from the test matrix and fixed the 2 errors from my previous pull request. Thank you for your help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I revised the two test files to include the errors without the version check. I also removed the older ruby versions ( < 2.7) from the test matrix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @nickmendezFlatiron this is great! I believe there is a duplicated block to be removed. Could you remove it and then we can approve this? Thanks!
@nickmendezFlatiron if you run |
…rected offenses with rubocop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the suggested changes using rubocop and removed the duplicate code from the test file. Thank you for your help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2.5
and 2.6
)
Summary
I Completed the 3 tasks in Issue #2529
Other Information