Skip to content

Generate a condition when value is false for boolean predicates#335

Merged
jonatack merged 1 commit intoactiverecord-hackery:masterfrom
PChambino:inverse_predicates
Aug 27, 2014
Merged

Generate a condition when value is false for boolean predicates#335
jonatack merged 1 commit intoactiverecord-hackery:masterfrom
PChambino:inverse_predicates

Conversation

@PChambino
Copy link
Copy Markdown
Contributor

I noticed that for boolean predicates the value false is being ignored. This can be counterintuitive as I would expect that name_present = false would give me all records where name equals NULL or is empty.

I found this issue #9 and implemented something starting from @cawel spec.

I am not sure if it is the best approach, but let me know what you think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we need inverse_of_true and not_true and false. These seem like really confusing predicate names. Especially when you get to inverse_of_not_true. So... not true would be false, but the inverse of that would mean true? If so, then why not use the true predicate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the implementation is confusing. The inverse_of_... predicates are simply a way to accept false values. I can try to implemented it in another way, removing the need for these strange predicates. I also accept suggestions. :)

However, the not_true and false predicates are not equivalent if you consider NULL values. For example, the true predicate will only match true values, the false predicate will only match false values, but the not_true predicate will match false and NULL values, which is the real inverse of the true predicate. But this can be an issue/request on its own.

@jonatack
Copy link
Copy Markdown
Contributor

Thanks, @PChambino. I believe this issue has just been fixed yesterday, but your solution is interesting and different from the one merged in; I'll have a look at it and at your tests and see what could be merged in.

@PChambino
Copy link
Copy Markdown
Contributor Author

Yes, #370 already fixes this issue. I still think there are some missing aspects that should be considered.

  • I think it makes sense to make all "boolean" predicates (present, blank, null, not_null, true, false) behave the same way. So predicates true and false should also accept false values.
  • I also think that the false predicate is not really the opposite of the true predicate (as it says in the wiki) if you consider NULL values. So predicates not_true and not_false should be added.

What do you think of this two points?

@avit
Copy link
Copy Markdown
Contributor

avit commented Aug 11, 2014

I patched around this in our codebase by redefining not_eq and does_not_match as derived predicates.

Related: #123
Related: rails/arel#165

@PChambino
Copy link
Copy Markdown
Contributor Author

Updated to include not_true and not_false predicates for completeness.

jonatack added a commit that referenced this pull request Aug 27, 2014
Add `not_true` and `not_false` predicates
@jonatack jonatack merged commit c689a1f into activerecord-hackery:master Aug 27, 2014
@jonatack
Copy link
Copy Markdown
Contributor

Thanks Pedro! Could you add these predicates to the Basic Searching section in the wiki?

@PChambino
Copy link
Copy Markdown
Contributor Author

Sure! I took a shoot at it already. Added the false predicate, changed the opposite of the true predicate to not_true, and added a short note under the false predicate. What do you think?

@jonatack
Copy link
Copy Markdown
Contributor

Thanks! Seems good. I added a second sentence:

Note: the `false` predicate may be considered the opposite of the `true` predicate
if the field does not contain null values. Otherwise, use `not_false`.

Hope that is correct.

@PChambino
Copy link
Copy Markdown
Contributor Author

In the second sentence I was actually expecting to read:

Otherwise, use not_true.

But I guess it depends on what you think "Otherwise" is referring to. What do you think about rewriting it to something like:

Otherwise, use the not_true/not_false predicates accordingly.

@jonatack
Copy link
Copy Markdown
Contributor

I see. Maybe the explanation should be more explicit?

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