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

r.expr({a: 1}).merge([1,2,3]) #3110

Open
neumino opened this issue Sep 29, 2014 · 8 comments
Open

r.expr({a: 1}).merge([1,2,3]) #3110

neumino opened this issue Sep 29, 2014 · 8 comments

Comments

@neumino
Copy link
Member

neumino commented Sep 29, 2014

I accidently run something like:

r.expr({a: 1}).merge([1,2,3])

And it returns [1,2,3].

I would expect an error.

@neumino neumino added the tp:bug label Sep 29, 2014
@neumino neumino added this to the 1.15.x milestone Sep 29, 2014
@coffeemug
Copy link
Contributor

This definitely seems like a bug. Also note:

> r.expr({a:1}).merge(1)
1

@mlucy -- is this actually a bug or intended behavior? I'd like to hear what you think before we fix this (in case this was by design for a reason that doesn't come to mind).

@Tryneus
Copy link
Member

Tryneus commented Sep 29, 2014

datum_t::merge will always return the right-hand side in the case that not all arguments are objects. Not sure if this is intended, but it's pretty explicitly there in the code. Note that if we do change this, we'll need old and new version-handling for it.

@mlucy
Copy link
Member

mlucy commented Oct 8, 2014

I think that was the original intended behavior, but I would be fine with changing it. Re-tagging as a ReQL proposal and putting into subsequent in case anyone can remember a good reason why we wanted to do it that way.

@mlucy mlucy modified the milestones: subsequent, 1.15.x Oct 8, 2014
@timmaxw
Copy link
Member

timmaxw commented Oct 8, 2014

r.expr({key:{a:1}}).merge({key:[1,2,3]}) should obviously return {key:[1,2,3]}. The current behavior can be seen as a generalization of that to the case where the nesting level is zero. I speculate that that was the original reason. I would also be fine with changing it.

@mlucy
Copy link
Member

mlucy commented Oct 10, 2014

I can see why it's confusing, though, because:

  • r({a: {b: 1}}).merge({a: 2}) => {a: 2}
  • r({b: 1}).merge(2) => 2
  • r({a: 1}).merge({a: 2}) => {a: 2}
  • r(1).merge(2) => ERROR

@danielmewes danielmewes modified the milestones: 2.1, subsequent Mar 23, 2015
@danielmewes
Copy link
Member

This seems to be the same as #2270

@danielmewes danielmewes modified the milestones: 2.1, 2.1-polish Mar 26, 2015
@mlucy
Copy link
Member

mlucy commented Apr 22, 2015

I'm actually not so sure we should change this any more. I think instead we should maybe make r(1).merge(2) return 2.

@danielmewes
Copy link
Member

Sorry for realizing this only after you'd already commented, but I'm actually going to move this to subsequent as well (i.e. out of the 2.1 discussion period).

@danielmewes danielmewes modified the milestones: subsequent, 2.1-polish Apr 22, 2015
@danielmewes danielmewes modified the milestones: subsequent, backlog Apr 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants