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

Never return an array in JavaScript #604

Closed
neumino opened this issue Apr 4, 2013 · 12 comments
Closed

Never return an array in JavaScript #604

neumino opened this issue Apr 4, 2013 · 12 comments
Milestone

Comments

@neumino
Copy link
Member

neumino commented Apr 4, 2013

So I talked about that with @al3xandru few days ago. At that time, he somehow convinced me that it was ok to get back an array and not a cursor.

I still don't feel confortable getting an array back sometimes.
It's a pain because I have to write my callback in different ways and figure out what the good way to do it.

query.run( connection, function( err, results) {
    if err
        throw err
    util.log(results);
})

vs

query.run( connection, function( err, cursor) {
    if err
        throw err
    cursor.toArray( function(err, results) {
        if err
            throw err
        util.log(results);
    }
})

An argument to have array was for a query like

r.expr([1,2,3])

But that means this query is going to return an array too.

r.expr([1,2,3]).append( r.table("bar").map(r.row("id") )

If possible, every time we send back an array or a stream, we should provide an cursor.everytime

@neumino
Copy link
Member Author

neumino commented Apr 4, 2013

In Python I guess it's ok to get back an array since you iterate on an array or on a cursor the same way.

@coffeemug
Copy link
Contributor

I'm sorry, but I really don't want to reopen this topic. Despite looking similar, arrays and cursors are different things, and trying to provide the same interface to the two is a pandora's box. We actually managed to close this box last time. We had the strength to send the demons back once, I don't know if we'll be able to summon this much strength again.

(I do agree that it's inconvenient. We should probably allow explicit coerceTo('stream') in ReQL to let you get a cursor if you really want it.)

@neumino
Copy link
Member Author

neumino commented Apr 4, 2013

I don't remember having a topic about whether we send back an array or a cursor.
When did we decide to make a difference between a cursor and an array?

@coffeemug
Copy link
Contributor

This was discussed in a lot of depth in #221 and friends.

@neumino
Copy link
Member Author

neumino commented Apr 4, 2013

#221 mainly discuss about the style of the callbacks.
I don't see anywhere that the callback can be called on an array (except if an array is atomic?).

I'll leave this issue closed for now, but I'm really not convinced that we should keep it like that. From my point of view, it's currently and by far the most annoying thing in the new JavaScript driver.
I'll probably try to talk about that with you tomorrow.

@al3xandru
Copy link
Contributor

As I've mentioned in our private conversation, my view on this is that there are two types of results:

  1. objects (either an object or a list): this is returned from functions like db_create, table_create, & family, insert & family, get.
  2. cursor: for everything else.

@neumino
Copy link
Member Author

neumino commented Apr 4, 2013

So we talked with @coffeemug and @al3xandru and we agreed on is to add the methods next(), hasNext(), each() and toArray() on the result if the result is an array.

So users can write their callback in JavaScript in the same way whether they get back an array or a stream.

Reopening and assigning to @wmrowan

@neumino neumino reopened this Apr 4, 2013
@ghost ghost assigned neumino and wmrowan Apr 4, 2013
@wmrowan
Copy link
Contributor

wmrowan commented Apr 5, 2013

What's the priority on this?

@coffeemug
Copy link
Contributor

Mr. President comes first :)

@wmrowan
Copy link
Contributor

wmrowan commented Apr 5, 2013

And this second? There's no milestone.

On Fri, Apr 5, 2013 at 2:05 AM, coffeemug notifications@github.com wrote:

Mr. President comes first :)


Reply to this email directly or view it on GitHubhttps://github.com//issues/604#issuecomment-15945422
.

@coffeemug
Copy link
Contributor

Let's do everything currently slated for sprint-2 first. If there's still time, we can add this to sprint-2. Otherwise, sprint-3 (unless there are more important things to do).

@wmrowan
Copy link
Contributor

wmrowan commented Apr 19, 2013

This is in code review 413 by @neumino

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

No branches or pull requests

4 participants