Ransack for edge rails #1445
Ransack for edge rails #1445
Conversation
|
Interesting! I think the reason this code existed is that we needed different patches for the different versions that we supported. Sounds like things are more stable for us now and in our current support matrix these patches are exact duplicates of each other. If that's the case, can we instead rename the folder to "activerecord", and always load that, without any magic? |
|
Good idea, I'll whip up a follow up PR trying to implement the change to simplify things. |
|
I just noticed we may actually need your solution or something similar given #1447. |
|
Nice refactor thanks @tvongaza |
|
@deivid-rodriguez any idea why the CI is getting stuck? I've re-run it but it stopping in the same place. 
|
Thanks @tvongaza or feel free to push directly to this PR. We can probably merge it as-is, but it would be cleaner with the approach @deivid-rodriguez suggested. |
|
@tvongaza could you please rebase this?
|
|
I don't think we can merge this as is after #1447, since we now have a Rails 6.1 specific monkey patch again. What I think we can do is, have a main folder named "activerecord", and load that if a folder specific to current rails is not found. How about that? |
|
Ok, updated the branch with the latest changes. I did not see a 6.1 specific monkey patch, as again 7.0 and 7.1 both just call the 6.1 code. I moved it all to an |
|
https://github.com/activerecord-hackery/ransack/pull/1447/files changes a 6.1 specific monkey patch so these files should not be identical. |
But the 7.0 & 7.1 files just point to the 6.1 files - or am I missing something obvious? They all look like this: https://github.com/activerecord-hackery/ransack/blob/main/lib/polyamorous/activerecord_7.0_ruby_2/join_association.rb#L1 |
|
Oh, you're right I missed that! That's not ideal since for example in #1447 just to fix a Rails 6.1 specific issue, we ended up changing the monkey patches for every Rails version. However, since that's what we're doing already, this seems fine, and it allows people to experiment with Ransack against edge Rails, which is good! Can you remove the last commit though? I don't want to run CI in all PRs against a moving target like Rails edge, too unstable. We already run CI against Rails edge on a daily schedule. |
Oh nice, did realize the gem had a daily edge rails CI run. Commit removed. |
|
Ok, let's merge this. Thanks so much @tvongaza! |
|
Hello - could you please release 4.1.1 (?) with this fix? 4.1.0 fails with the following error, but ransack Thanks! |
4.1.1 has been released |
I'm actually not too sure why this code even exists. Rails officially only supports 6.1 as per https://guides.rubyonrails.org/maintenance_policy.html, maybe worth dropping support for older versions & just rolling these fixes in?
As I'm unsure of the side effects, this code just stops copying and pasting directories, instead load the last known good version and try to use that.