Skip to content

Conversation

timoschilling
Copy link
Member

In following to #3486 (comment) from @seanlinsley

The next release is going to be 1.0.0, and we'll be following semantic versioning. I really don't want that cruft in the code base until 2.0.0

We should remove the metasearch backports.

@seanlinsley
Copy link
Contributor

I backported these because I liked the old naming conventions better. contains is much easier to understand than cont, for example.

@timoschilling
Copy link
Member Author

Yes that's true, I even don't like the Ransack names. But we should not support both. The problem is we have a dirty mix, we support some metasearch matcher, but not all and we support ransack but only under the hut. I understand you (@seanlinsley) that you want a clean version 1.0.0 (#3486 (comment)). That we should make a backport and use this backport as default, is confusing for people that look at the at the ransack doc (including me).

@seanlinsley
Copy link
Contributor

The situation here is that Ransack has a different opinion on naming conventions than Metasearch did. I find Metasearch's conventions to be much easier to read and understand. I don't think we should bend to Ransack's conventions simply because we're relying on their code.

The problem is we have a dirty mix, we support some metasearch matcher, but not all and we support ransack but only under the hut.

We can add them.

That we should make a backport and use this backport as default, is confusing for people that look at the at the ransack doc (including me).

Then we should document the fact that these custom names aren't built into Ransack.

@timoschilling
Copy link
Member Author

We should not write a mapper around Ransack, we should use Ransack or Metasearch not both!
If we are using Ransack pure, we can get some benefits:

  1. A clean API
  2. Simple documentation, we can say "We use ransack, for details take a look on ransack docs"
  3. We can remove the predecates keys from I18n and switch to use ransack locales and don't need to maintain our one. This counts event for custom filter made by the user, the needs to translate thy twice, one time in the ransacker namespace for a frontend and a sound time for ActiveAdmin
  4. The user can clearly see whats happen and don't run into issues while it looks like metasearch but is not metasearch

@timoschilling timoschilling force-pushed the remove_metasearch_backports branch from 6c3f252 to 032f2df Compare November 13, 2014 22:26
@timoschilling timoschilling force-pushed the remove_metasearch_backports branch from 032f2df to 34a7eb8 Compare December 7, 2014 02:02
@pboling
Copy link

pboling commented Apr 25, 2015

Would it be possible to also get active_admin to loosen up the dependency on ransack as well? Ransack is already past a 1.6.x release, but the dependency definition in the gemspec here requires a 1.3.x version.

We use ransack in our app outside of active_admin, and would like to be able to upgrade it independently (within reason). You are using the between concept for the rails dependency with s.add_dependency 'rails', '>= 3.2', '< 5.0', so perhaps ransack could be:

s.add_dependency 'ransack',             '>= 1.3', '< 2.0'

@timoschilling
Copy link
Member Author

@pboling ~> 1.3 requires a version 1.x not a 1.3.x, so ~>1.3 is the same as your '>= 1.3', '< 2.0'. The Rails dependency is defined as '>= 3.2', '< 5.0' to allow 3.2.x and 4.x.

@pboling
Copy link

pboling commented Apr 25, 2015

Oh, right. I knew that at one point. Thanks.

@seanlinsley
Copy link
Contributor

Before my nearly year-long hiatus, I had been working on AA for around two full years, during which I made a ton of backwards-incompatible changes without actually making a release. Now that I'm back, I'm very hesitant to make any more breaking changes unless they're necessary.

I still prefer verbs like contains versus cont, but more importantly people other than myself have been relying on this for almost two years now.

@timoschilling
Copy link
Member Author

@seanlinsley I give you right about the "contains versus cont", but ransack name it con so we should not rename it. For more reasons read my previous comment #3511 (comment). Why do we need backports at this place?

@seanlinsley
Copy link
Contributor

but ransack name it con so we should not rename it.

I assume this was a typo, and you meant to type cont

As I said, other than my preferring the more human contains, the other reason to keep it is those that ported their AA apps from Metasearch to Ransack two years ago were at least able to keep those verb names the same. Now you're asking them to change their code again, when we haven't even made a stable release since the Metasearch -> Ransack migration was completed two years ago.

And besides this being a breaking change, it doesn't affect the readability or performance of our code. So this change should at the very least wait until version 2.0.0.

I'm sorry, but we're wasting our time here arguing about this. There are much more important issues to resolve.

I'm going to close this PR. We can come back to it once we start considering what breaking changes to include in 2.0.0.

@seanlinsley seanlinsley closed this May 4, 2015
@seanlinsley seanlinsley deleted the remove_metasearch_backports branch May 4, 2015 14:33
@timoschilling timoschilling restored the remove_metasearch_backports branch August 17, 2015 16:58
@timoschilling
Copy link
Member Author

I reopen this PR, while the backports are buggy, see #4078, #4094 for example

@timoschilling timoschilling reopened this Aug 17, 2015
@seanlinsley seanlinsley added this to the 2.0.0 milestone Feb 18, 2016
@javierjulio
Copy link
Member

@timoschilling since you had this for v2, are you still interested in getting this out? Would @Fivell be able to help out here? I've noticed a lot of his work with these predicates and such.

@deivid-rodriguez deivid-rodriguez force-pushed the remove_metasearch_backports branch from 34a7eb8 to 7e65f38 Compare December 10, 2018 11:26
@deivid-rodriguez
Copy link
Member

Not yet passing but I rebased this PR!

@deivid-rodriguez deivid-rodriguez force-pushed the remove_metasearch_backports branch from 7e65f38 to b72b1e9 Compare December 11, 2018 07:22
@deivid-rodriguez deivid-rodriguez modified the milestones: 2.0.0, 3.0.0 Sep 19, 2019
@javierjulio
Copy link
Member

@deivid-rodriguez do you think we could still get this in? Maybe easier to create a new PR and redo the changes rather than resolve each merge conflict?

javierjulio added a commit that referenced this pull request Jul 11, 2023
This closes #3511 and is updated branch with no merge conflicts. Instead of fixing each conflict, since the original PR is very old, started from scratch making each change and running the tests.

This removes backports for a gem that was deprecated in favor of ransack. We want to reduce our footprint and just rely on ransack's defaults and docs.
@javierjulio
Copy link
Member

This will be addressed in #8010 and part of our next major release around Ransack changes.

@javierjulio javierjulio deleted the remove_metasearch_backports branch July 11, 2023 01:53
javierjulio added a commit that referenced this pull request Jul 12, 2023
This closes #3511 and is updated branch with no merge conflicts. Instead of fixing each conflict, since the original PR is very old, started from scratch making each change and running the tests.

This removes backports for a gem that was deprecated in favor of ransack. We want to reduce our footprint and just rely on ransack's defaults and docs.
javierjulio added a commit that referenced this pull request Jul 12, 2023
This closes #3511 and is updated branch with no merge conflicts. Instead of fixing each conflict, since the original PR is very old, started from scratch making each change and running the tests.

This removes backports for a gem that was deprecated in favor of ransack. We want to reduce our footprint and just rely on ransack's defaults and docs.
javierjulio added a commit that referenced this pull request Jul 13, 2023
Remove metasearch backport, use ransack defaults

This closes #3511 and is updated branch with no merge conflicts. Instead of fixing each conflict, since the original PR is very old, started from scratch making each change and running the tests.

This removes backports for a gem that was deprecated in favor of ransack. We want to reduce our footprint and just rely on ransack's defaults and docs.
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.

5 participants