Mongo package linting #8175

dr-dimitru · 2016-12-21T00:29:55Z

mongo_driver.js:

Lines 66, 1011 and 1352: Linting
Line 497: return after throw
Lines 523 - 527: e already defined in same function scope (hoisting)
Line 937: Missed semicolon
Line 1095: Very confused about ES6 “default parameter” set via colon, which probably will be interpreted as Object - Babel failed to build on my end, regarding this docs it should be a Boolean, so we set it to false by default
Line 1120: no need to set variable to undefined
Lines 1122, 1128, and 1133: Define doc above while due to function scope and hoisting doc is already allocated inside while and try

polling_observe_driver.js

newResults defined inside try, and used in parent scope later

oplog_tailing.js

lastEntry defined on while loop (line 139), and used later at parent scope

oplog_observe_driver.js

Line 425 and 448: maxBuffered is defined twice in same scope (hoisting)

mongo_livedata_tests.js

Lines 481 - 498: val and x is defined twice (hoisting)
Lines 973 - 975 and 980 - 982: expectedRemoves and expectedAdds is already defined on lines 952 and 955

allow_tests.js

Line 16: No need to set variable to undefined

package.js

Beta version bump

Other files

Overall linting, missed semicolons

- Line 497: `return` after `throw` - Lines 523 - 527: `e` already defined in same function scope (hoisting) - Line 937: Missed semicolon - Line 1095: Very confused about ES6 “default parameter” set via colon, which probably will be interpreted as Object - Babel failed to build on my end, regarding [this docs](https://mongodb.github.io/node-mongodb-native/api-generated/cursor .html#count) it should be a Boolean, so we set it to `false` by default - Line 1120: no need to set variable to `undefined` - Lines 1122, 1128, and 1133: Define `doc` above while due to function scope and hoisting `doc` is already allocated inside `while` and `try`

- Lines 481 - 498: `val` and `x` is defined twice (hoisting) - Lines 973 - 975 and 980 - 982: `expectedRemoves` and `expectedAdds` is defined on lines 952 and 955

- Line 425 and 448: `maxBuffered` is defined twice in same scope (hoisting)

- `lastEntry` defined on while loop (line 139), and used later at parent scope

- `newResults` defined inside try, and used in parent scope later

benjamn · 2017-01-10T23:52:03Z

What linter ruleset are you using here? The long lines from compound conditions seem like a step backwards for readability, though other changes seem like good ideas.

dr-dimitru · 2017-01-11T01:59:15Z

Hi @benjamn ,

I'm using SublimeLinter + Babel + eslint with very default setup.

From .eslintrc:

    "rules": {
        "indent": [
            "error",
            2
        ],
        "linebreak-style": [
            "error",
            "unix"
        ],
        "quotes": [
            "off",
            "single"
        ],
        "semi": [
            "error",
            "always"
        ],
        "no-console": [
            "off", 
            {
                "allow": ["warn", "error", "info", "log", "trace"]
            }
        ]
    }

I can change it back on your demand, changed because even on my 15" screen it has space for those lines, and looks different to me. I guess it depends from developer to developer.

abernix

Thanks for trying to fix these details, @dr-dimitru. Your linter configuration is a bit bare to use as a lint config for Meteor and would conflict with some of the "official" rules, but I think there are some reasonable changes in here. I've made suggestions that I think would make this mergeable.

For future reference, the most recently prescribed Meteor linter rules were https://github.com/meteor/javascript (a fork of https://github.com/airbnb/javascript), although it is arguably a bit out of date and needs some merging with some of the more recent additions to the Airbnb version. I'll try to address that soon, but you might want to check out the .eslintrc for Meteor which is supposed to match that guide. (Though you should also see the .eslintignore which excludes many files, intentionally. 😉

Thanks again for this. I do think cleanup like this is valuable in incremental amounts.

abernix · 2017-01-13T17:30:12Z

packages/mongo/collection_tests.js

@@ -89,7 +89,7 @@ Tinytest.add('collection - call native find with sort function',
    if (Meteor.isServer) {
      test.throws(
        function () {
-          nativeCollection.find({}, {sort: function(){}}).map(function (doc) { return doc.a; })
+          nativeCollection.find({}, {sort: function(){}}).map(function (doc) { return doc.a; });


Could you also go ahead and wrap/indent this while we're at it?

nativeCollection .find({}, { sort: function () {}, }) .map( doc => doc.a );

abernix · 2017-01-13T17:57:08Z

packages/mongo/mongo_driver.js

-    throw new Error("Can't observe an " + (ordered ? "ordered" : "unordered")
-                    + " tailable cursor without a "
-                    + (ordered ? "addedBefore" : "added") + " callback");
+    throw new Error("Can't observe an " + (ordered ? "ordered" : "unordered") + " tailable cursor without a " + (ordered ? "addedBefore" : "added") + " callback");


Preference to revert this. Previous readability was better and lines over 100 characters are too much. While there may be room with your IDE maximized, not everyone does this.

abernix · 2017-01-13T18:16:57Z

packages/mongo/observe_changes_tests.js

-                    + (forceOrdered ? " force ordered" : ""),
-                    function (test, onComplete) {
+
+  Tinytest.addAsync("observeChanges - single id - basics " + added + (forceOrdered ? " force ordered" : ""), function (test, onComplete) {


Prefer to revert.

abernix · 2017-01-16T16:35:10Z

packages/mongo/oplog_observe_driver.js

-      var maxBuffered = (limit && self._unpublishedBuffer.size() > 0)
-        ? self._unpublishedBuffer.get(self._unpublishedBuffer.maxElementId())
-        : null;
+      var maxBuffered = (limit && self._unpublishedBuffer.size() > 0) ? self._unpublishedBuffer.get(self._unpublishedBuffer.maxElementId()) : null;


Prefer to revert.

abernix · 2017-01-16T16:58:11Z

packages/mongo/mongo_livedata_tests.js

-    return other instanceof TestCustomType
-      && EJSON.equals(this.myHead, other.myHead)
-      && EJSON.equals(this.myTail, other.myTail);
+    return other instanceof TestCustomType && EJSON.equals(this.myHead, other.myHead) && EJSON.equals(this.myTail, other.myTail);


Preference to revert.

abernix · 2017-01-16T16:58:44Z

packages/mongo/oplog_observe_driver.js

-      var maxBuffered = (limit && self._unpublishedBuffer.size() > 0)
-        ? self._unpublishedBuffer.get(self._unpublishedBuffer.maxElementId())
-        : null;
+      var maxBuffered = (limit && self._unpublishedBuffer.size() > 0) ? self._unpublishedBuffer.get(self._unpublishedBuffer.maxElementId()) : null;


Revert. 🙂

abernix · 2017-01-16T18:01:41Z

packages/mongo/mongo_driver.js

@@ -1092,7 +1090,7 @@ _.extend(SynchronousCursor.prototype, {
    return self.map(_.identity);
  },

-  count: function (applySkipLimit: false) {
+  count: function (applySkipLimit = false) {


- Changes according to PR request - Apply rule [meteor#16.1](https://github.com/meteor/javascript#16.1) to if/else blocks

- Line 88: `selector` variable defined but never used - Apply rule [meteor#16.1](https://github.com/meteor/javascript#16.1) to if/else blocks

- Apply rule [meteor#16.1](https://github.com/meteor/javascript#16.1) to if/else blocks

mongo_driver.js: - Line 609: Argument `driverResult` in `transformResult` is always a number - Line 639: `errmsg` property is not exists on `err` object - Line 735 & 758: `result` argument of `bindEnvironmentForWrite` is always a *Number* oplog_tailing.js: - Line 217: `setName` property of `isMasterDoc` is not exists - Line 218: Error message is not correct, as it’s not same as on line 185 remote_collection_driver.js: - No need to define `self` variable here

dr-dimitru · 2017-01-17T12:58:55Z

Hi @abernix ,

Thank you for detailed review. Required changes is pushed.

For some reason I can not run tests for this package unless changes is made, please take a look on this commit. What do you think?

abernix · 2017-01-17T15:51:15Z

packages/mongo/mongo_driver.js

      } catch (err) {
        // There's no good way to figure out if this was actually an error
        // from Mongo. Ah well. But either way, we need to retry the cursor
        // (unless the failure was because the observe got stopped).
-        doc = null;
+        // `doc` is already `null` here;


I don't think this change is safe or just a "linting" change – at least not without further consideration or explanation.

This is in a loop. While doc is null the first time around, if cursor._nextObject throws an error, it would be left as the previous doc that was processed on this cursor (loop). A tailable cursor remains open until additional documents arrive.

Do you disagree?

@abernix you're right. My bad.

abernix · 2017-01-17T15:52:41Z

I don't see how veliovgroup@a513097 is related to this PR. Is there an actual issue that you're trying to fix with that commit?

dr-dimitru · 2017-01-17T18:05:59Z

veliovgroup@a513097 not really related to this PR.

It's popped-out as I tried to run meteor test-packages ./, of course including MONGO_URL and MONGO_OPLOG_URL. Anyways I'm able to run and pass most of tests only after changes at this commit, should I start separate ticket/PR?

abernix · 2017-01-18T10:37:03Z

@dr-dimitru Ok, this LGTM now. Hopefully someone else can take a look over it too and 👍 it.

Regarding veliovgroup@a513097: yes, let's make that a separate issue which shows the problem you're encountering, including specifically which tests are failing. Also, try to keep each commit/PR to one logical change. That being said, the "fix" reminds me of something that turned up in #7758 (comment) (re: errmsg vs err) which we never resolved. The full test suite is a bit tricky at times – you may find solace in looking at scripts/ci.sh in the Meteor repo to see what the CI suite runs.

Older error objects is: ```js {name: String, code: Number, errmsg: String} ``` Newer mongo return error Object as: ```js {name: String, code: Number, err: String} ``` This commit adds support for new error Object without dropping support for older. - Mentioned as confirmed bug at: meteor#7758 - Partly discussed at: meteor#8175 - Originally fixed [at this commit](a5130970bc6f79af683 ba2fd10dd4402ef2cdb6c)

benjamn · 2017-01-18T16:15:04Z

packages/mongo/collection_tests.js

-          nativeCollection.find({}, {sort: function(){}}).map(function (doc) { return doc.a; })
+          nativeCollection
+            .find({}, {
+              sort: function () {},


Though I'm not going to request changes, in the future you should know you can write methods like this using method syntax:

nativeCollection .find({}, { sort() {}, })

Hi @benjamn ,
Thank you for merge.
I wish to use ES7 syntax, but most of file seems to be ES5. Am I wrong?

The Mongo error property, `errmsg`, has been changed to `err` on newer versions of Mongo. This commit adds support for the new property without dropping support for older Mongo versions. Partially addresses #7758 (comment) Discussed in #8175 (comment)

dr-dimitru added 11 commits December 21, 2016 02:50

Merge remote-tracking branch 'meteor/devel' into devel

9df15fc

beta version bump

349a1b4

No need to set variable to undefined

cea6f4a

Overall listing, missed semicolons

a71a6e6

Missed semicolons

fa631c4

Add missed semicolons, one-line misleading lines

1472a41

Add missing semicolons, fix variables scope

4197c9d

- Lines 481 - 498: `val` and `x` is defined twice (hoisting) - Lines 973 - 975 and 980 - 982: `expectedRemoves` and `expectedAdds` is defined on lines 952 and 955

Fix misleading lines and variable scope

7a8ce9a

- Line 425 and 448: `maxBuffered` is defined twice in same scope (hoisting)

Variable used out of scope

92a2c4f

- `lastEntry` defined on while loop (line 139), and used later at parent scope

Variable used about of scope

23b5564

- `newResults` defined inside try, and used in parent scope later

abernix suggested changes Jan 16, 2017

View reviewed changes

dr-dimitru added 8 commits January 17, 2017 12:09

Changes according to PR request

bd61f6b

Fiber lib is not used at mongo_driver.js file

3739a60

Requested changes + apply some JavaScript Style Guide rules

a60b30f

- Changes according to PR request - Apply rule [meteor#16.1](https://github.com/meteor/javascript#16.1) to if/else blocks

Requested changes + JavaScript Style Guide rules

ada34ef

- Changes according to PR request - Apply rule [meteor#16.1](https://github.com/meteor/javascript#16.1) to if/else blocks

Changes as requested

fc3349b

Fiver packages is not used

2a8667d

More linting

5b193e5

- Line 88: `selector` variable defined but never used - Apply rule [meteor#16.1](https://github.com/meteor/javascript#16.1) to if/else blocks

Changes as requested + JavaScript Style Guide rules

ebde43d

- Apply rule [meteor#16.1](https://github.com/meteor/javascript#16.1) to if/else blocks

abernix reviewed Jan 17, 2017

View reviewed changes

PR review changes

edf58bc

abernix approved these changes Jan 18, 2017

View reviewed changes

dr-dimitru mentioned this pull request Jan 18, 2017

Fix Mongo Error Message Object #8266

Merged

benjamn reviewed Jan 18, 2017

View reviewed changes

benjamn merged commit d93c021 into meteor:devel Jan 18, 2017

abernix mentioned this pull request Jan 19, 2017

Syntax error in Mongo Driver? #7929

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mongo package linting #8175

Mongo package linting #8175

dr-dimitru commented Dec 21, 2016 •

edited

benjamn commented Jan 10, 2017

dr-dimitru commented Jan 11, 2017

abernix left a comment

abernix Jan 13, 2017

abernix Jan 13, 2017

abernix Jan 13, 2017

abernix Jan 16, 2017

abernix Jan 16, 2017

abernix Jan 16, 2017

abernix Jan 16, 2017

dr-dimitru commented Jan 17, 2017

abernix Jan 17, 2017

dr-dimitru Jan 17, 2017

abernix commented Jan 17, 2017

dr-dimitru commented Jan 17, 2017

abernix commented Jan 18, 2017

benjamn Jan 18, 2017

dr-dimitru Jan 18, 2017

Mongo package linting #8175

Mongo package linting #8175

Conversation

dr-dimitru commented Dec 21, 2016 • edited

benjamn commented Jan 10, 2017

dr-dimitru commented Jan 11, 2017

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-dimitru commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abernix commented Jan 17, 2017

dr-dimitru commented Jan 17, 2017

abernix commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-dimitru commented Dec 21, 2016 •

edited