Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Remove duplicate property definition, also allows code to be used in strict mode. #916

Merged
merged 1 commit into from

2 participants

@ohjames

No description provided.

@ohjames

Sorry about that, guidelines say to ensure an issue is open but it seems github now automatically creates an issue for each pull request. Maybe I made a dumb mistake or maybe that guideline is redundant?

@christkv
Owner

can you provide a test case for the problem this solves please

@ohjames

Aren't we over engineering a little here, surely a duplicate key is an obvious minor deletion that can be made. The test for this would depend on the underlying node engine's support of strict mode which would tie the test case to specific versions of node. Whilst normally I also agree with an ultra-paranoid approach to testing I believe if ever something didn't warrant a test case that this is it. If you do not care about strict mode support or the extra few bytes in the file please feel free to ignore this.

@ohjames

Commendable attention to detail though! I get accused of the same thing by my colleagues :package:

@christkv
Owner

"is more this also allows code to be used in strict mode" that I'm confused about not the duplicate message.

@ohjames

In Ecmascript 6 there is a new strict mode that can be enabled by putting "use strict"; either at the beginning of the global scope or at the beginning of a function. This allows the javascript compiler to more aggressively optimise as it disables some historical relics that prevent aggressive optimisations.

One of those historical relics is support for duplicate keys in object constructors. I think maybe it's because they conflict with the (I believe non-specified) notion that VMs should currently maintain key ordering.

You could put a test for this case but it would rely on the version of node being used as support for "use strict" has not always been there, so the test would lead to a false positive in earlier versions of node. I mean I could be done I suppose... but really the duplicate key to me is just an obvious neglect of the eye that should be fixed regardless of strict mode. Hope that helps, sorry for spending so much time rambling instead of just providing the test case, next time!

@christkv
Owner

ah that script mode. I was confused as there is a strict mode operation on creation of collections.

@ohjames

What an understandable misunderstanding!

@christkv christkv merged commit cd9a2d1 into mongodb:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 22, 2013
  1. @ohjames
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 1 deletion.
  1. +1 −1  lib/mongodb/collection.js
View
2  lib/mongodb/collection.js
@@ -758,7 +758,7 @@ Collection.prototype.findAndRemove = function(query, sort, options, callback) {
var testForFields = {
limit: 1, sort: 1, fields:1, skip: 1, hint: 1, explain: 1, snapshot: 1, timeout: 1, tailable: 1, tailableRetryInterval: 1
, numberOfRetries: 1, awaitdata: 1, exhaust: 1, batchSize: 1, returnKey: 1, maxScan: 1, min: 1, max: 1, showDiskLoc: 1
- , comment: 1, raw: 1, readPreference: 1, numberOfRetries: 1, partial: 1, read: 1, dbName: 1
+ , comment: 1, raw: 1, readPreference: 1, partial: 1, read: 1, dbName: 1
};
/**
Something went wrong with that request. Please try again.