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

Remove Rails 4.2 support #1422

Merged
merged 1 commit into from
Mar 4, 2021
Merged

Remove Rails 4.2 support #1422

merged 1 commit into from
Mar 4, 2021

Conversation

vsppedro
Copy link
Collaborator

@vsppedro vsppedro commented Mar 2, 2021

I think we don't need this comment:

module Kernel
# #capture, #silence_stream, and #silence_stderr are deprecated after Rails
# 4.2 and will be removed in 5.0, so just override them completely here

But I'm not sure if I should do anything other than remove it.

@mcmire
Copy link
Collaborator

mcmire commented Mar 2, 2021

@vsppedro I think the method is worth keeping, because we make use of it, but perhaps the comment should be updated so that instead of talking about something that will happen, it talks about something that already did happen?

@vsppedro
Copy link
Collaborator Author

vsppedro commented Mar 3, 2021

@vsppedro I think the method is worth keeping, because we make use of it, but perhaps the comment should be updated so that instead of talking about something that will happen, it talks about something that already did happen?

Something like this?

module Kernel
# #capture, #silence_stream, and #silence_stderr were removed in rails 5.0,
# but we keep it them here

@mcmire
Copy link
Collaborator

mcmire commented Mar 4, 2021

@vsppedro Yeah, that works for me.

@vsppedro
Copy link
Collaborator Author

vsppedro commented Mar 4, 2021

@mcmire, would you like me to change anything else in this PR? Just to make sure if I can merge as it is. 😅

Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

@vsppedro Yeah, I guess I wasn't sure how you were going about this, but it looks like your plan is just to remove Rails 4.2 from CI in this PR and do cleanup around removing all of the places where we're checking for Rails 4.2 in other PRs, right? If that's the case then this looks good to me.

@vsppedro
Copy link
Collaborator Author

vsppedro commented Mar 4, 2021

Yes, that is exactly what I'm thinking of doing. Thanks! Merging!

@vsppedro vsppedro merged commit edf4f5d into master Mar 4, 2021
@vsppedro vsppedro deleted the remove-rails-4-2 branch March 4, 2021 22:43
@vsppedro vsppedro mentioned this pull request Jun 4, 2021
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.

2 participants