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

[299] Let attribute names include "and" / "or" #302

Conversation

seanlinsley
Copy link
Contributor

This is my attempt at fixing #299. It works, but likely doesn't cover all scenarios.

The /_and_|_or_/ style logic exists all over the place; for example I know the translation code is still broken in the context of #299. Maybe it'd be better to extract the attribute splitting logic out to a single method that all code would use.

@avit
Copy link
Contributor

avit commented Nov 14, 2013

The mysql builds are failing... One scenario this won't cover is foo_and_bar_and_baz where the attributes are foo_and_bar, baz.

@avit
Copy link
Contributor

avit commented Sep 4, 2014

I don't think this is workable unless we change internals to __and__ with double-underscores or such.

Suggest closing this.

@seanlinsley
Copy link
Contributor Author

I think it'd be perfectly fine to use __and__ internally. Or, we could pass around a hash of queries that itself has already been separated for or/and conditions.

@avit
Copy link
Contributor

avit commented Sep 4, 2014

Parsed conditions hashes are usable already if you set up your own nested groups of and/or, etc. We should probably document that better in the wiki or provide some helpers for building more complex conditions.

I wouldn't recommend changing the current _and_ format for flat hashes since that could break a ton of client code. (Unless we decide to plan for it with a major breaking version.)

@jonatack
Copy link
Contributor

@seanlinsley Thanks again for this contribution.

I tried your above test to let attribute names include "and" / "or" on the latest Ransack -- and it still fails (see commit 5f8621c today).

So we are not there yet...

@seanlinsley
Copy link
Contributor Author

Shouldn't this have been resolved by #449?

@seanlinsley seanlinsley deleted the bugfix/299-let-attributes-use-and/or branch October 27, 2014 21:19
@jonatack
Copy link
Contributor

Apparently not. The attribute is detected, but the search is not picking up the attribute.

@jonatack
Copy link
Contributor

I tried your test today (see the last commit), and it isn't passing on Ransack master.

@seanlinsley seanlinsley restored the bugfix/299-let-attributes-use-and/or branch October 27, 2014 21:27
@seanlinsley seanlinsley reopened this Oct 27, 2014
@timoschilling
Copy link
Contributor

This PR is should be replaced by #562 (which is merged) and can be closed

@jonatack
Copy link
Contributor

Thanks @timoschilling.

@jonatack jonatack closed this Aug 18, 2015
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.

4 participants