-
-
Notifications
You must be signed in to change notification settings - Fork 910
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 unnecessary conditions for rails 5.0 #1426
Conversation
cbc7fa1
to
59fee8a
Compare
Hi, @mcmire, I hope you're doing fine. When you have some free time, would you mind taking a look at this PR and sharing your thoughts on it, please? I think we're almost finishing removing all the support for EOL'ed Ruby and Rails releases. 🎉 |
I found another file that has some conditionals that we no longer need. I will remove these methods and update this PR as soon as possible. shoulda-matchers/lib/shoulda/matchers/rails_shim.rb Lines 6 to 12 in e746f2e
shoulda-matchers/lib/shoulda/matchers/rails_shim.rb Lines 20 to 22 in e746f2e
|
7ecab33
to
4ed36ba
Compare
@vsppedro Okay, sounds good! |
4ed36ba
to
02b61dd
Compare
0bac7de
to
76ea384
Compare
I think that's it. I can't find any more conditionals that need to be removed for now. What do you think? And after the merge of this PR, it seems to me that the next should be the: Remove Rails 5.0 support. |
lib/shoulda/matchers/rails_shim.rb
Outdated
@@ -58,12 +46,7 @@ def generate_validation_message( | |||
end | |||
|
|||
def make_controller_request(context, verb, action, request_params) |
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.
Now that there is no logic in make_controller_request
, tables_and_views
, and validation_message_key_for_association_required_option
, what do you think about folding this code into the place where they are used so that we can remove these methods?
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 think this is a good idea! I'll do that asap. Thanks!
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.
Done!
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.
Very nice! Looks great!
Thanks! |
This PR is a continuation of the Refactor ActiveRecordVersions- #1423.
I'll keep this as a draft meanwhile the other PR is not merged.