Skip to content

Include Calculations module instead of prepending #928

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

Merged
merged 1 commit into from
Jul 29, 2021

Conversation

ehutzelman
Copy link
Contributor

Another gem (groupdate: https://github.com/ankane/groupdate by @ankane) also patches the ActiveRecord::Relation#calculate method and breaks when used with this library. Prepending this Calculations module prevents any other libraries from patching this method, and it looks like an include will have the same desired effect to patch existing out-of-the-box ActiveRecord code. I'm not sure if this is prepended to ensure nothing else is patching #calculate, and how important that may be.

I see the reasoning for patching #calculate by this library is helpfully defined in the patched method:
# Same as original except we don't perform PostgreSQL hack that removes ordering.

Comparing to the original activerecord #calculate implementation, the following lines are removed for count operations:

# PostgreSQL: ORDER BY expressions must appear in SELECT list when using DISTINCT
relation.order_values = [] if group_values.empty?

In my somewhat obscure edge case scenario where both of these gems are used (along with postgres and sqlserver), I need to choose which gem is going to win the monkey patching war for #calculate. Changing this from prepend to include allows me to make this choice by the sequence the gems are loaded in. The downside of this change is it could allow another gem to patch #calculate away from the intended version in this library (but hopefully that is not much of a threat, and maybe a feature, as it is in my case).

@wpolicarpo
Copy link
Member

Can I ask you to rebase your branch so CI can run again?

Another gem (groupdate) also patches the #calculate method
and prepending this module prevents any other libraries from
patching this method.
@wpolicarpo wpolicarpo merged commit f35f47e into rails-sqlserver:main Jul 29, 2021
lavika pushed a commit to lavika/activerecord-sqlserver-adapter that referenced this pull request Sep 26, 2023
Another gem (groupdate) also patches the #calculate method
and prepending this module prevents any other libraries from
patching this method.
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