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

Jump to bottom

Make coerceTo("BOOL") work on all types? #3133

Closed
danielmewes opened this issue Oct 1, 2014 · 9 comments
Closed

Make coerceTo("BOOL") work on all types? #3133

danielmewes opened this issue Oct 1, 2014 · 9 comments
Labels
tp:API_settled
Milestone
2.2

Comments

@danielmewes
Copy link
Member

Currently we have:

r.expr(null).coerceTo('BOOL')
    RqlRuntimeError: Cannot coerce NULL to BOOL in:
r.expr(0).coerceTo('BOOL')
    RqlRuntimeError: Cannot coerce NUMBER to BOOL in:
r.expr("false").coerceTo('BOOL')
    RqlRuntimeError: Cannot coerce STRING to BOOL in:
r.expr("foo").coerceTo('BOOL')
    RqlRuntimeError: Cannot coerce STRING to BOOL in:
r.expr([]).coerceTo('BOOL')
    RqlRuntimeError: Cannot coerce ARRAY to BOOL in:
r.expr({}).coerceTo('BOOL')
    RqlRuntimeError: Cannot coerce OBJECT to BOOL in:

It seems that coerceTo('BOOL') basically doesn't do anything at all. Is that correct?

Here are three options of what we could do about it:

  1. Leave it as it is now, without a real use
  2. Make it consistent with r.branch(x, true, false), i.e.
r.expr(null).coerceTo('BOOL')
    false
r.expr(0).coerceTo('BOOL')
    true
r.expr("false").coerceTo('BOOL')
    true
r.expr("foo").coerceTo('BOOL')
    true
r.expr([]).coerceTo('BOOL')
    true
r.expr({}).coerceTo('BOOL')
    true

'3. Try to be a bit "smarter" with respect to numbers and strings:

r.expr(null).coerceTo('BOOL')
    false
r.expr(0).coerceTo('BOOL')
    false
r.expr(1).coerceTo('BOOL')
    true
r.expr(50).coerceTo('BOOL')
    ERROR: Only 0 and 1 can be coerced to BOOL.
r.expr("").coerceTo('BOOL')
    false (?)
r.expr("false").coerceTo('BOOL')
    false
r.expr("true").coerceTo('BOOL')
    true
r.expr("foo").coerceTo('BOOL')
    ERROR: Only "true" and "false" can be coerced to BOOL.
r.expr([]).coerceTo('BOOL')
    true
r.expr({}).coerceTo('BOOL')
    true
@danielmewes danielmewes added this to the 1.16-polish milestone Oct 1, 2014
@neumino
Copy link
Member

neumino commented Oct 2, 2014

Making things consistent with the behavior of r.branch sounds a bit better to me.
The rule being only false and null will evaluate to false. All the other values being evaluated to true.

Making something smarter for numbers seems dangerous. It looks like a first step to what JavaScript does when == is involved.

@thelinuxlich
Copy link

+1 for neumino's proposal, Javascript falsy sux :)

@VeXocide
Copy link
Member

VeXocide commented Oct 2, 2014

For strings, arrays, and objects I'm in favour of a Python inspired solution where it's essentially based on the length:

r.expr("").coerceTo('BOOL')
    false
r.expr("foo").coerceTo('BOOL')
    true
r.expr([]).coerceTo('BOOL')
    false
r.expr(["foo", "bar"]).coerceTo('BOOL')
    true
r.expr({}).coerceTo('BOOL')
    false
r.expr({"foo": "bar"}).coerceTo('BOOL')
    true

@timmaxw
Copy link
Member

timmaxw commented Oct 2, 2014

I think consistency with r.branch is the most important thing. Otherwise I'm indifferent between the three proposals.

@danielmewes
Copy link
Member Author

I'm sold for variant 2, consistency with r.branch.

It seems that while we could make a "smart" behavior consistent with some language, it would always be inconsistent with many others. In the end it seems like it will just behave unexpectedly for most users.

@danielmewes danielmewes modified the milestones: subsequent, 1.16-polish Oct 16, 2014
@thelinuxlich thelinuxlich mentioned this issue Nov 13, 2014
Closed
@danielmewes danielmewes modified the milestones: 2.1, subsequent Mar 23, 2015
@danielmewes danielmewes mentioned this issue Mar 30, 2015
Closed
@mlucy
Copy link
Member

mlucy commented Apr 22, 2015

I agree we should go for consistency with r.branch.

@VeXocide
Copy link
Member

I'm not sold on how r.branch currently handles it, but I agree we should go for consistency.

@danielmewes
Copy link
Member Author

Marking as settled for making coerceTo('BOOL') consistent with .branch(true, false).

@danielmewes danielmewes mentioned this issue May 12, 2015
Open
@danielmewes danielmewes modified the milestones: 2.2-polish, 2.2 Aug 27, 2015
eliangidoni added a commit to eliangidoni/rethinkdb that referenced this issue Oct 25, 2015
@eliangidoni
Fix for rethinkdb#3133
71c2c9c
danielmewes added a commit that referenced this issue Nov 2, 2015
@danielmewes
Merge pull request #4996 from eliangidoni/fix-3133
3901e28 
Fix for #3133
@danielmewes
Copy link
Member Author

Fixed by @eliangidoni in next: #4996
Will ship with 2.2.0

@danielmewes danielmewes modified the milestones: 2.2-polish, 2.2 Nov 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
tp:API_settled
Projects
None yet
Milestone
2.2
Development

No branches or pull requests
6 participants
@thelinuxlich @VeXocide @timmaxw @danielmewes @neumino @mlucy