-
-
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
Refactor ActiveRecordVersions #1423
Conversation
@vsppedro Yes, this makes perfect sense! I love the smell of deleted code... 😂 |
@mcmire, would you like this PR to be divided into smaller parts? One for each method removed in ActiveRecordVersion? I'm asking this because this PR got too big. |
38ec065
to
9db871b
Compare
I'll add a commit for every method removed, so you can check them separately if you like. |
9db871b
to
cd0ee6a
Compare
bbcd43f
to
07b0670
Compare
@vsppedro That's fine with me. I don't mind a long PR as long as everything is all related to the same idea. Separate commits sounds good though. |
Done! I'm thinking of merging this PR first, after your review. After that the next to be merged I think it should be Refactor UnitUnitTests::RailsVersions. After both being merged, I think we can go with Remove Rails 5.0 support. Would you like a different approach? |
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.
Sorry I didn't catch that you finished this a few days ago. Looks good to me!
No problems! If you like, I can add you as a reviewer after finishing a PR. Maybe that way it’s easier to know what’s ready. What do you think? |
Yeah that makes sense. I monitor notifications on GitHub pretty frequently throughout the day, so I think it should show up then.
Yeah that sounds good to me. I've taken a look at both PRs so far and they both make sense so I'd say your plan is good! |
Thanks! Merging now! |
This PR is a continuation of the
Remove Rails 4.2 support
- #1422.I'll keep this as a draft meanwhile the other PR is not merged.
I realized that we don't need some active record validations in the tests. The purpose of this PR is to remove conditionals that are no longer needed due to the current version that we support.
What do you think?