Skip to content
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

Merged
merged 2 commits into from
Aug 13, 2017
Merged

style(tests): mixed lint updates #1939

merged 2 commits into from
Aug 13, 2017

Conversation

layershifter
Copy link
Member

This PR contains only styling changes, it was splitted from #1895 to simplify review.

@codecov-io
Copy link

codecov-io commented Aug 7, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@ae448ae). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae448ae...e80288c. Read the comment docs.

'It was called with args:\n' +
JSON.stringify(handlerSpy.args, null, 2)
'It was called with args:\n'}${
JSON.stringify(handlerSpy.args, null, 2)}`,
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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)
Copy link
Member

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.

Copy link
Member

@levithomason levithomason left a 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.

@layershifter
Copy link
Member Author

@levithomason this come from airbnb rules, I gave link in a comment:

If the function body consists of a single statement returning an expression without side effects, omit the braces

@levithomason
Copy link
Member

That would refer to the curly braces of the function body{}, I'm referring to the parentheses of the arguments ().

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.

@layershifter
Copy link
Member Author

@levithomason This code was automatically fortmatted with eslint and airbnb preset. I split #1895 to smaller PRs only for review purposes.

It looks confusing, but it's correct. Take a look 8.2, this paragraph affects arrow-parens rule, too.

// 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);

Copy link
Member Author

@layershifter layershifter left a 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.

@levithomason
Copy link
Member

Got it, thank you again. I'm OK with this so long as it is fully automated. 👍

@levithomason levithomason merged commit 4f36ccd into master Aug 13, 2017
@levithomason levithomason deleted the style/test-mixed branch August 13, 2017 16:57
levithomason pushed a commit that referenced this pull request Aug 20, 2017
* 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
@levithomason
Copy link
Member

Released in semantic-ui-react@0.71.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants