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

Make cursor.map/forEach follow the ECMAScript APIs #63

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@iwoj

iwoj commented Apr 19, 2012

...as per convention in Javascript. This makes for much more elegant code.

Ian Wojtowicz
Added an index parameter to the cursor.forEach callback, as per conve…
…ntion. Makes for much nicer application code.
meteor Outdated
@@ -20,7 +20,7 @@ if [ "$UNAME" = "Darwin" ] ; then
fi
ARCH="x86_64"
elif [ "$UNAME" = "Linux" ] ; then
ARCH=$(uname -m)
ARCH="$(uname -m)"

This comment has been minimized.

@iwoj

iwoj Apr 19, 2012

This may not be necessary.

meteor Outdated
@@ -36,7 +36,7 @@ if [ -L "$(basename $0)" ] ; then
cd $(dirname $(readlink $(basename "$0") ) )
fi
SCRIPT_DIR=$(pwd -P)
cd $ORIG_DIR
cd "$ORIG_DIR"

This comment has been minimized.

@iwoj

iwoj Apr 19, 2012

This is crucial for installing in directories with spaces.

Ian Wojtowicz added some commits Apr 19, 2012

Ian Wojtowicz
Completely updated cursor.map and cursor.forEach to mimic official EC…
…MAScript Array.forEach and Array.map functions. Their callbacks now both have optional index and traversal object parameters.
Ian Wojtowicz
Fixed a small but significant mistake with the thisArg object being p…
…assed to the callback function. It shouldn't affect the self variable.
@AndyMoreland

This comment has been minimized.

AndyMoreland commented Apr 19, 2012

I'm not sure how strict the maintainers of this project are, but personally I'd prefer to see the patches to environment variables separated from the change to forEach.

@iwoj

This comment has been minimized.

iwoj commented Apr 20, 2012

Good point. It's not clear how to submit them separately. How do I do that?

@AndyMoreland

This comment has been minimized.

AndyMoreland commented Apr 20, 2012

My mistake -- I meant the bash variable changes.

edit: also, I completely agree with those changes being applied somehow.

@debergalis

This comment has been minimized.

Member

debergalis commented Apr 24, 2012

I think this is a good idea, with some changes.

1: The API should follow the full ECMA 5 spec for map and forEach:

  • in addition to callback they should both take a second, optional thisArg.
  • they should call callback with three arguments: the document, the index of that document, and the Cursor.
  • inside the callback, this should be bound to thisArg, if provided, otherwise the global object associated w/ callback.

2: The same API should be on the server, as implemented in packages/mongo-livedata/mongo_driver.js.

3: Tests for client and server APIs. (packages/minimongo/minimongo_tests.js and packages/mongo-livedata/mongo_livedata_tests.js).

@debergalis

This comment has been minimized.

Member

debergalis commented Apr 24, 2012

(The patch should be based against devel, not master. When you rebase against devel the diffs to the meteor script will go away -- they've been fixed.)

@iwoj

This comment has been minimized.

iwoj commented Apr 25, 2012

Matt: I attended to #1 and #2. I also rebased my branch, but I'm not sure if I did this correctly.

As for the tests, I'm not sure how your unit test framework functions. Can you point me in the right direction?

@debergalis

This comment has been minimized.

Member

debergalis commented Apr 25, 2012

@holmsand what non-standard behavior do you mean?

@holmsand

This comment has been minimized.

holmsand commented Apr 25, 2012

@debergalis Sorry I was unclear...

But it seems to be quite fundamental in Coffeescript to protect you from accidentally creating global variables (see http://coffeescript.org/#lexical_scope). As far as I know, the "bare" option is mainly aimed at e.g. Node's module loader, that provides it own wrapper anyway.

@debergalis

This comment has been minimized.

debergalis commented on 2b74157 Apr 25, 2012

How'd you rebase? I'd have expected $ git rebase origin/devel to work. But this looks more like a merge. Anyone know how to rebase a pull request against a new branch?

Why do you say self.cursor_pos starts at 1? It's just like any array offset. Also, let's pass the Cursor as the third argument, following ECMA.

So in forEach, would this work? I prefer avoiding the extra variable.

while (self.cursor_pos < self.db_objects.length) {
  if (thisArg === undefined)
    callback(LocalCollection._deepcopy(self.db_objects[self.cursor_pos]), self.cursor_pos, self);
  else
    callback.call(thisArg, LocalCollection._deepcopy(self.db_objects[self.cursor_pos]), self.cursor_pos, self);
  self.cursor_pos++;
}

In map, we can use the new index.

self.forEach(function (doc, index) {
  if (thisArg === undefined)
    res.push(callback(doc, index, self));
  else
    res.push(callback.call(thisArg, doc, index, self));
});

But it's early and I haven't had my coffee yet.

For testing: see the test suite in packages/minimongo/minimongo_tests.js. There's a section testing map and forEach which you should expand to cover all these different cases. To run the test suite, run meteor inside packages/minimongo and visit localhost:3000 just like any other meteor app. You'll see all the test results in green, and as you make changes to the tests or the code, the results will live update in the browser.

This comment has been minimized.

Owner

iwoj replied Apr 28, 2012

Yes, I merged by accident. I'm still learning my way around git. Sorry for the mess. I've fixed the cursor_pos thing. I'm not sure why I thought this was necessary.

I've made the two changes you suggested as well. As for the testing, I'll starting looking at it soon. In the meantime, if anyone has any suggestions for how I can pull those merges our of this pull request, I'd be very obliged.

Thanks.

@debergalis

This comment has been minimized.

Member

debergalis commented Apr 25, 2012

@holmsand I see, you're commenting on a separate change that got added to the pull request from the rebase. #85 (and #33) is a better place for the discussion.

@ghost ghost assigned glasser Sep 7, 2012

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 18, 2012

@iwoj: It might help a lot if you were to do the changes again against the current devel branch (not against master), this way you can squash the few amount of changes you have into a single commit as well as making it easier for them to pull in. So, in short:

  1. Pull meteor's devel branch into a new feature branch on your repo.
  2. Manually bring over the changes from this pull request.
  3. Do a pull request against meteor's devel branch.

@debergalis: Why did you refer to those two issues, was that by accident? I don't see how they are relevant.

@debergalis

This comment has been minimized.

Member

debergalis commented Sep 19, 2012

They're not -- I was referring to an earlier comment.

We'd take something close to the rebased patch if the API is tested on both client and server. I didn't see that in the earlier diff.

@n1mmy n1mmy referenced this pull request Aug 19, 2013

Closed

Index in cursor.forEach #1345

glasser added a commit that referenced this pull request Sep 24, 2013

Implements ES5-style callbacks for cursor forEach and map.
Fixes #63.

Based on iwoj's PR.

Needs tests and docs.

@glasser glasser closed this in d4d7ebb Sep 28, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment