-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
style(tests): mixed lint updates #1939
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1939 +/- ##
=========================================
Coverage ? 99.77%
=========================================
Files ? 148
Lines ? 2637
Branches ? 0
=========================================
Hits ? 2631
Misses ? 6
Partials ? 0 Continue to review full report at Codecov.
|
'It was called with args:\n' + | ||
JSON.stringify(handlerSpy.args, null, 2) | ||
'It was called with args:\n'}${ | ||
JSON.stringify(handlerSpy.args, null, 2)}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, looks like the update here went awry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, should be fixed. Will fix tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this.
@@ -26,7 +26,7 @@ describe('numberToWord', () => { | |||
}) | |||
|
|||
it('returns word if input is word', () => { | |||
words.forEach((word) => numberToWord(word).should.equal(word)) | |||
words.forEach(word => numberToWord(word).should.equal(word)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it that in some (most?) cases the parens are added, but in others they are removed?
..etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -9,7 +9,7 @@ const testsContext = require.context('./', true, /-test\.js$/) | |||
// only re-run changed tests, or all if none changed | |||
// https://www.npmjs.com/package/karma-webpack-with-fast-source-maps | |||
const __karmaWebpackManifest__ = [] | |||
let runnable = testsContext.keys().filter((path) => __karmaWebpackManifest__.indexOf(path) >= 0) | |||
let runnable = testsContext.keys().filter(path => __karmaWebpackManifest__.indexOf(path) >= 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's another example where parens are removed while most others are added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is syntax error in isConformant.js
, see inline comment.
Other than this, it seems the style is also inconsistent between files (arrow func arg parens). That said, I'd rather implement eslint
rules that fixable for these so we don't have to manage them ourselves.
@levithomason this come from airbnb rules, I gave link in a comment:
|
That would refer to the curly braces of the function body When there is a single argument, the changes here arbitrarily add parens in some cases and remove them in other cases. I can't seem to figure out what the reason is. tests.bundle.js (Removes parens) -.filter((path) =>
+.filter(path => test/specs/views/Feed/FeedEvent-test.js (Adds parens) -contentProps.forEach(propKey => {
+contentProps.forEach((propKey) => { test/specs/lib/numberToWord-test.js (Removes parens) -words.forEach((word) =>
+words.forEach(word => test/specs/behaviors/Visibility/Visibility-test.js (Adds parens) -expectations.forEach(expectation => {
+expectations.forEach((expectation) => { ...there are many of these in the PR. |
@levithomason This code was automatically fortmatted with It looks confusing, but it's correct. Take a look 8.2, this paragraph affects // bad
[1, 2, 3].map(number => {
const nextNumber = number + 1;
`A string containing the ${nextNumber}.`;
});
// good
[1, 2, 3].map(number => `A string containing the ${number}.`);
// good
[1, 2, 3].map((number) => {
const nextNumber = number + 1;
return `A string containing the ${nextNumber}.`;
});
// good
[1, 2, 3].map((number, index) => ({
[index]: number,
})); Paragraph 8.4 also describes behaviour of parentheses. // bad
[1, 2, 3].map((x) => x * x);
// good
[1, 2, 3].map(x => x * x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levithomason I've fixed formatting of that message and answered about parens.
Got it, thank you again. I'm OK with this so long as it is fully automated. 👍 |
* chore(package): update all dependencies * style(mixed): fix lint issues * chore(package): update deps, add es6 support to configs * style(docs): revert Table and Menu changes, splitted to #1938 * style(docs): revert tests changes, splitted to #1939 * style(mixed): restore formatting * style(mixed): revert tests changes, splitted to #1949 * style(configs): ESify configs * chore(package): update deps * style(mixed): fix lint issues * style(eslint): cleanup configs * fix(configs): fix import of webpack config * chore(package): update package-lock.json
Released in |
This PR contains only styling changes, it was splitted from #1895 to simplify review.