Upgrade eslint & config to latest #1179
Conversation
- eslint ^3.11.1 - eslint-config-loopback: ^6.0.0 - fix linter errors (mostly no-undef)
|
The failed downstream builds are unrelated AFAICT - two timeouts and an SQL error. |
| // This test written in mocha+should.js | ||
| 'use strict'; | ||
|
|
||
| /* global getSchema:false, connectorCapabilities:false */ |
There was a problem hiding this comment.
We should fix (ie. not use) these globals at some point. ;) That is outside the scope of this PR though.
There was a problem hiding this comment.
I agree with both! Should we create an issue to keep track of this work? I am not sure if this problem important enough, there is no point in creating issues that nobody will work for months.
There was a problem hiding this comment.
I believe it is important to keep our house as clean as possible and as soon as possible. We should not delay these minor issues for months IMO, but I'll leave that decision up to @ritch. Issue created at #1186 and assigned to @siddhipai
| // to guarantee to create it in the same turn of even loop | ||
| return self._createSync(model, data, function(err, id) { | ||
| if (err) return process.nextTick(function() { cb(err); }); | ||
| if (err) return process.nextTick(function() { callback(err); }); |
There was a problem hiding this comment.
Why not cb? I still prefer cb over callback unless we've decided to do this all over the place.
There was a problem hiding this comment.
@bajtos @superkhau either one is fine with me; consistency is what I would follow... I did a quick search in lib/connectors/memory.js and it seems most of the methods use callback, so I would personally go for callback in this case; however I agree if I had the option with not lots of callbacks in the file, I would go for cb rather than callback but it is a nitpick.
@superkhau this one actually was a bug since there was no cb in this method and that's why @bajtos changed it to callback.
There was a problem hiding this comment.
I'm also fine with either -- I would just like some consistency in our codebase instead of both.
Bug makes sense, but I would like the bugfix in a seperate commit at least so we know its unrelated to the ESLint fixes.
There was a problem hiding this comment.
In this patch, I am fixing undefined cb to the correct callback. Let's keep the discussion and (possibly) the refactoring out of scope of this pull request please.
There was a problem hiding this comment.
It's a nitpick anyways, 2 commits (one for bug and one for the eslint changes) would've been better for the reviewer to determine WHY the cb/callback thing was changed since the bugfix is not a requirement of ESLint since this PR was for ESLint changes. I'm not saying make a separate PR, just another commit on top of the current changes.
| // to guarantee to create it in the same turn of even loop | ||
| return self._createSync(model, data, function(err, id) { | ||
| if (err) return process.nextTick(function() { cb(err); }); | ||
| if (err) return process.nextTick(function() { callback(err); }); |
There was a problem hiding this comment.
@bajtos @superkhau either one is fine with me; consistency is what I would follow... I did a quick search in lib/connectors/memory.js and it seems most of the methods use callback, so I would personally go for callback in this case; however I agree if I had the option with not lots of callbacks in the file, I would go for cb rather than callback but it is a nitpick.
@superkhau this one actually was a bug since there was no cb in this method and that's why @bajtos changed it to callback.
|
Landed, thank you! |
@superkhau or @Amir-61 please review