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 ActiveRecord::Relation#calculate patch #1198

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

jonahgeorge
Copy link
Contributor

@jonahgeorge jonahgeorge commented Jul 2, 2024

Hey all,

I've been working on adding SQL Server support to ankane/groupdate and discovered that this patch of ActiveRecord::Relation#calculate is interfering with the library's own patching of the same method.

It appears that all tests pass without this patch so posting this PR to potential remove it as I don't fully follow why it was introduced to start.

@aidanharan aidanharan merged commit 1beb276 into rails-sqlserver:main Jul 2, 2024
4 checks passed
@aidanharan
Copy link
Contributor

aidanharan commented Jul 2, 2024

The ActiveRecord::Relation#calculate patch was introduced by #1090 to fix an issue with failing activerecord/test/cases/null_relation_test.rb tests. Not sure exactly what tests now and since the tests pass now without the patch then all good 👍

@jonahgeorge The main branch is used for Rails 7.2 development. Do you need the patch removed from the 7.1 adapter?

@jonahgeorge
Copy link
Contributor Author

The ActiveRecord::Relation#calculate patch was introduced by #1090 to fix an issue with failing activerecord/test/cases/null_relation_test.rb tests. Not sure exactly what tests now and since the tests pass now without the patch then all good 👍

@jonahgeorge The main branch is used for Rails 7.2 development. Do you need the patch removed from the 7.1 adapter?

Would you accept backports for both 7.1 and 7.0? Unfortunately the app that I really need this in is still on 7.0-- No worries if not

@jonahgeorge jonahgeorge deleted the remove-calculate-patch branch July 3, 2024 00:18
@aidanharan
Copy link
Contributor

Backports welcome 👍

@aidanharan
Copy link
Contributor

@jonahgeorge Released v7.0.7 and v7.1.4 with the back ports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants