Add Airbnb JavaScript Style Guide for eslint #28
Conversation
| Array.from(attributes) | ||
| .forEach(({ name, value }) => { | ||
| result[name] = value; | ||
| }); |
There was a problem hiding this comment.
Q: was this change for the linter? the reduce is a lot simpler to me personally
There was a problem hiding this comment.
indeed, the results[name] = value; line triggered the no-param-reassign rule
if we add the transform-object-rest-spread babel plugin, we could instead write something like this:
return Array.from(attributes)
.reduce((results, { name, value }) => {
return {
...results,
[name]: value
};
}, {});alternatively, we could use Object.assign in a similar way 🤔
i'll come up with a more legible option
There was a problem hiding this comment.
no i'm ok with your solution. no need! you can just move along
test/XMLToReact.test.js
Outdated
| import React from 'react'; | ||
| /* eslint-disable no-unused-expressions */ | ||
|
|
||
| import { describe, it, before, beforeEach, after } from 'mocha'; |
There was a problem hiding this comment.
these aren't accepted as globals using the airbnb styleguide? (i guess we can't target just .spec files in a config. that's a little annoying but i guess we'll live with it.
test/XMLToReact.test.js
Outdated
| @@ -1,6 +1,10 @@ | |||
| import React from 'react'; | |||
| /* eslint-disable no-unused-expressions */ | |||
There was a problem hiding this comment.
let's not disable rules. let's use the function form e.g. .to.equal(true) versus .to.be.true
test/XMLToReact.test.js
Outdated
| @@ -10,92 +14,94 @@ import { visitNode } from '../src/helpers'; | |||
| configure({ adapter: new Adapter() }); | |||
|
|
|||
| describe('XMLToReact class ', () => { | |||
| const TestComponent = ({ fancy, children }) => { | |||
| const classes = fancy ? 'test fancy' : 'test'; | |||
| return React.createElement('div', { className: classes }, children); | |||
There was a problem hiding this comment.
trivial... you could name the variable className and save yourself a few characters ;)
.eslintrc.js
Outdated
| mocha: true, | ||
| browser: true, | ||
| node: true, | ||
| es6: true |
There was a problem hiding this comment.
do we reference anything browser-specific where we need to specify browser?
also, do we need es6 here in addition to ecmaVersion above?
There was a problem hiding this comment.
re: browser, nope! i felt that adding it here may be safer if we ever need to start using globals in a browser environment, but we can always add it when necessary
re: es6, it seems to enable the same features as ecmaVersion with the exception of modules.
i'll leave mocha as the only environment
|
🚀 shipping, as all feedback has been addressed and this is looking safe to merge. |
This work enables a solid set of rules for linting the source code in the project.
Description
While this project is set up to use eslint for linting our JavaScript source code, it did not have any rules enabled to be checked against.
This work sets up eslint to use the Airbnb JavaScript Style Guide, and fixes issues to make the code adhere to these rules.
Types of changes
Related Issue
#23
#15
Motivation and Context
The project requires a solid base of eslint rules in which it should adhere.
How Has This Been Tested?
This was tested by running our linter and unit test suites, and ensured that all functionality remains.
Checklist: