Update eslint-config-airbnb-base to version 12.0.0 🚀 #997
Conversation
![greenkeeper[bot]](https://avatars.githubusercontent.com/in/505?s=60&v=4){kind=small}
Also fixes a minor 🐛 in `helpers.js` where the given message wasn't being used properly.
Arcanemagus
force-pushed
the
greenkeeper/eslint-config-airbnb-base-12.0.0
branch
from
acfc65d
to
8618651
Compare
Update ESLint to a minimum of v4.6.0 to satisfy the `peerDependencies` of `eslint-config-airbnb-base`.
| @@ -63,9 +64,10 @@ export async function sendJob(worker, config) { | |||
| export function showError(givenMessage, givenDetail = null) { | |||
| let detail | |||
| let message | |||
| if (message instanceof Error) { | |||
There was a problem hiding this comment.
Note that this was actually a 🐛, it was only checking the newly declared message, not the one passed in.
In fixing the earlier bug, a new one had been introduced where it was attempting to access a `msg` property of the `givenMessage`, which doesn't exist.
src/helpers.js
Outdated
| detail = message.stack | ||
| message = message.message | ||
| if (givenMessage instanceof Error) { | ||
| ({ stack: detail, message } = givenMessage) |
There was a problem hiding this comment.
Since this syntax is uncommon, it might be worthwhile adding a comment explaining what it's doing and maybe even linking to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Assignment_without_declaration.
There was a problem hiding this comment.
I'd say the syntax is uncommon only because a lot of people don't know about it, I'll certainly be using it in a lot more places as the only reason I haven't been doing things like that is I didn't know the syntax.
So the question is: Comment it when the end result is obvious, or just start using it everywhere so people see it?
There was a problem hiding this comment.
I'm not so sure the end result is exactly obvious, and destructuring can be confusing if you're not used to it, with this form even more so. Up to you though, I don't care very much.
| @@ -63,9 +64,10 @@ export async function sendJob(worker, config) { | |||
| export function showError(givenMessage, givenDetail = null) { | |||
| let detail | |||
| let message | |||
| if (message instanceof Error) { | |||
| @@ -61,9 +61,10 @@ describe('Worker Helpers', () => { | |||
| }) | |||
|
|
|||
| describe('getESLintInstance && getESLintFromDirectory', () => { | |||
| const pathPart = Path.join('testing', 'eslint', 'node_modules') | |||
There was a problem hiding this comment.
Why do we need to break out a separate variable for this? It adds some indirection and complexity, and I'm not sure I see the benefit.
There was a problem hiding this comment.
The lines below where this chunk is used were over the 100 char limit otherwise, and the previous method of breaking it into multiple lines doesn't fit the current style guide. I'm open to alternatives though!
src/worker.js
Outdated
| response = fixJob({ cliEngineOptions, contents, eslint, filePath }) | ||
| response = fixJob({ | ||
| cliEngineOptions, contents, eslint, filePath | ||
| }) |
There was a problem hiding this comment.
This (and others like it) is a fairly annoying change, since it breaks a single, understandable line into three lines. What linting rule is requiring it? Do you like having object literals always broken apart like this?
There was a problem hiding this comment.
It's being controlled by object-curly-newline, and I'm not sure I like it.
The current configuration of this rule can be found here. The critical part looking to be the minProperties: 4 bit, which we are just hitting on almost every case.
Should we override that rule here? Setting that to 5 allows almost all of the current object literals to remain as they are.
There was a problem hiding this comment.
Yeah, I think I'd be tempted to set it to 5, or just turn minProperties off and only set "consistent": true. I'm not really sure I see the benefit of setting a minProperties at all.
To be clear, I'm proposing:
'object-curly-newline': ['error', {
ObjectExpression: { consistent: true },
ObjectPattern: { consistent: true }
}],What do you think?
There was a problem hiding this comment.
Tried it out, I think I like that, our 4 level uses aren't that bad, and just setting that to 5 allows it to still catch bigger ones.
|
@IanVS did a review of the current ESLint config and changed some things. |
Arcanemagus
changed the title
|
LGTM |
greenkeeper
bot
deleted the
greenkeeper/eslint-config-airbnb-base-12.0.0
branch
Version 12.0.0 of eslint-config-airbnb-base just got published.
The version 12.0.0 is not covered by your current version range.
Without accepting this pull request your project will work just like it did before. There might be a bunch of new features, fixes and perf improvements that the maintainers worked on for you though.
I recommend you look into these changes and try to get onto the latest version of eslint-config-airbnb-base.
Given that you have a decent test suite, a passing build is a strong indicator that you can take advantage of these changes by merging the proposed change into your project. Otherwise this branch is a great starting point for you to work on the update.
Not sure how things should work exactly?
There is a collection of frequently asked questions and of course you may always ask my humans.
Your Greenkeeper Bot 🌴