Feature/eslint config by gavinorland · Pull Request #77 · stevesims/govuk-frederic

gavinorland · 2018-08-17T10:47:07Z

Hey guys, so here I have removed numerous ESLint exemptions, and brought all code into alignment accordingly. I have also changed the glob pattern for ESLint in the package.json, as /**/ was not detecting files in the root of each package!

If you have any views on the remaining overrides please let me know. If approved, perhaps we can copy this for govuk-react. Later also perhaps think about further steps such as running it on commit (although I know people have had issues with this) and possibly introducing Prettier. This addresses #5.

codecov · 2018-08-17T10:54:35Z

Codecov Report

Merging #77 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #77   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          22     22           
  Lines         185    185           
  Branches       42     43    +1     
=====================================
  Hits          185    185

Impacted Files	Coverage Δ
components/countdown-text-area/src/index.js	`100% <ø> (ø)`	⬆️
components/arrow-left/src/index.js	`100% <ø> (ø)`	⬆️
components/result-count/src/index.js	`100% <ø> (ø)`	⬆️
components/remove-button/src/index.js	`100% <ø> (ø)`	⬆️
components/keyline/src/index.js	`100% <ø> (ø)`	⬆️
components/count/src/index.js	`100% <ø> (ø)`	⬆️
components/open-button/src/index.js	`100% <100%> (ø)`	⬆️
components/counter-bar/src/index.js	`100% <100%> (ø)`	⬆️
components/result-count-title/src/index.js	`100% <100%> (ø)`	⬆️
components/table-of-contents/src/Section/index.js	`100% <100%> (ø)`	⬆️
... and 10 more

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 8bd5693...15a7afa. Read the comment docs.

stevesims · 2018-09-03T10:48:42Z

components/array-object-table/src/stories.js

  const defaultTransform = value => (value || '-');

-  return <ArrayObjectTable
+  return (<ArrayObjectTable


the parens here really needed?

i get the need for a bracket when there's nothing else on the line, but as this line also has the start of a JSX tag surely that's OK?

On current config react/jsx-wrap-multilines objects without these - I'll take a look at options...

ok cool - i think i'm not a fan of that rule then 😁

OK... There is no option enforcing that JSX is never wrapped in parens but I could turn off the rule requiring that multiline JSX is wrapped.

Wrapping is not necessary for multi-line JSX, but is advised by FB and Airbnb, FB saying it "avoids the pitfalls of automatic semicolon insertion". Shall we say that we will turn off the rule, though? This will mean there can be parens around both single and multiline JSX, but it is not required or "fixed" to be so. And our policy could be not to do it at all. That's fine with me.

btw If there is a return statement then a line break before the JSX, then yes, a bracket is needed or the return is deemed illegal, but I guess we could generally avoid this too and always begin with our JSX or opening element.

stevesims · 2018-09-03T10:49:16Z

components/array-object-table/src/stories.js

  const fields = [
    { key: 'one', heading: 'One' },
-    { key: 'two', heading: 'Two', transform: value => value ? value.toLowerCase() : '' },
+    { key: 'two', heading: 'Two', transform: value => (value ? value.toLowerCase() : '') },


not sure i see the value in the brackets here

This one is "Arrow function used ambiguously with a conditional expression" (no-confusing-arrow). Again will take a look at options...

yeah - as i said this one i'm not sure about - there's potentially value in that rule i guess, especially if the code in the ternary gets more complex and spread over multiple lines

Hm.. yes, there isn't any multi-line setting. We could override this rule if you like (turn it off)?

stevesims

some things to change

i didn't reach the end of this whilst reviewing - many of my comments have knock-on implications and would likely get repeated a lot as i continued through this PR

stevesims · 2018-09-03T10:51:20Z

components/arrow-left/src/index.js

@@ -1,5 +1,6 @@
 import React from 'react';
 import PropTypes from 'prop-types';
+


i don't see the need to have a line break here - react-emotion is a module that's needed for this component to work, and it's effectively the same type of fundamental module as react and prop-types to me

stevesims · 2018-09-03T10:54:06Z

components/compact-table-accordion-group/src/stories.js

 stories.addDecorator(withKnobs);

-stories.add('Component default', () => <CompactTableAccordionGroup title="Title" expanded="expanded">
+stories.add('Component default', () => (<CompactTableAccordionGroup title="Title" expanded="expanded">


once again, not sure why there's a bracket here

Ah, that one's been given a bracket because it's multiline JSX. I'm going to override that rule now though.

stevesims · 2018-09-03T10:55:12Z

components/compact-table-accordion-group/src/stories.js

+</CompactTableAccordionGroup>));

-examples.add('No children', () => <CompactTableAccordionGroup open title="Title" expanded="expanded"/>);
+examples.add('No children', () => <CompactTableAccordionGroup open title="Title" expanded="expanded" />);


and given the propensity for brackets elsewhere, why does this not have one?

indeed for me as I'm cool with this line not bracketing the return value, this example makes the case that the others also shouldn't need brackets

This was not given a bracket by ESLint because it's only on a single line. :)

components/compact-table-accordion-group/src/test.js

stevesims · 2018-09-03T10:56:42Z

components/compact-table-accordion-group/src/test.js

+    expect(wrapper
+      .find('Title')
+      .childAt(0)
+      .text()).toBe('Title');


... than this, as here it's jolly easy to miss the closing ) after .text()

This one is due to function-paren-newline. Will look at options on this one too..

stevesims · 2018-09-03T10:58:05Z

components/compact-table-accordion-group/src/test.js

    const changeHandler = jest.fn();
-    const wrapper = mount(
-      <CompactTableAccordionGroup expanded="Expanded" onChange={changeHandler}>
+    const wrapper = mount(<CompactTableAccordionGroup expanded="Expanded" onChange={changeHandler}>


i'd have left these myself - having the <CompactTableAccordionGroup start on a fresh line makes it a bit easier to read what the mount call is rendering when skimming the code

stevesims · 2018-09-03T11:02:49Z

components/counter-bar/src/index.js

+ *   <CounterBar.Total
+ *    score={15}
+ *    scoreColor="yellow"
+ *    scoreBackgroundColor="pink">All counters</CounterBar.Total>


if you're going to split the CounterBar.Total opening tag across multiple lines, then IMHO it's best to put the content and closing tags in separate lines to make the code more reasonable

plus also the indent should be kept consistent, i.e. attributes should go 2 characters in

i.e. this should be:

* <CounterBar.Total * score={15} * scoreColor="yellow" * scoreBackgroundColor="pink" * > * All counters * </CounterBar.Total>

Thanks. This one isn't an ESLint auto-fix!

stevesims · 2018-09-03T11:03:13Z

components/counter-bar/src/index.js

+ *     <CounterBar.Counter
+ *      score={2}
+ *      scoreColor="orange"
+ *      scoreBackgroundColor="blue">Counter 2</CounterBar.Counter>


similar formatting comments apply here

and indeed through the rest of these examples

stevesims · 2018-09-03T11:05:56Z

components/counter-bar/src/stories.js

    </CounterBar.Counters>
-  </CounterBar>),
-);
+  </CounterBar>));


i'm really not keen on having the closing bracket here for the stories.add call at a different indent level - it really grates for me when i'm reading this code, and some editors may try to maintain the 4 spaces when writing the next line of code

i.e. the closing bracket should go onto the next line, as it was before

indeed the old code here was fine, except it should not have had the trailing ,

this comment applies to the rest of this file too

This commits removes most of the core overrides, meaning Airbnb's rules apply once again.

This makes all the files respect the new ESLint rules. It's mostly adjusting line lengths (100 limit) and removing whitespace. I have also tried to make blank lines between import blocks uniform.

There was a surplus `/**/` in here, preventing these scripts from finding the files.

There seems to be a problem with the "zero or more folders" glob so it was missing out root files. Have got around that - we can consider regex in the future. Files here are corrected accordingly.

All code fixed to conform.

This branch has also been rebased onto the latest master, meaning this should be done before further edits are made.

`function-paren-newline` and `no-confusing-arrow` both get disabled here, we meaning don't need parens around JSX or around the ternaries in arrow functions. Also, `comma-dangle` is added with a special option which enables us to have our JSX blocks always properly aligned with no trailing comma and a closing paren on a separate line. Changes to code are: * Brackets removed in numerous places around JSX blocks where not required * When JSX lines are very long or when components are mounted with children they generally begin on their own new line * No trailing commas after JSX and closing parens on next line * Fixes to tabbing of children * `max-len` seems not to be taken into account by ESLint, but I have moved children onto a new line when line length would exceed 100 chars and generally kept to this rule elsewhere too * No blank line between packages which are core ones, but kept one between core ones and our ones, and between our ones and Storybook ones

gavinorland requested review from stevesims and taranchauhan August 17, 2018 10:47

gavinorland requested a review from 7oque August 20, 2018 08:22

stevesims reviewed Sep 3, 2018

View reviewed changes

stevesims requested changes Sep 3, 2018

View reviewed changes

gavinorland added 7 commits September 3, 2018 14:09

Remove core ESLint overrides

b20dd3b

This commits removes most of the core overrides, meaning Airbnb's rules apply once again.

Make codebase comply with ESLint

cd03fef

This makes all the files respect the new ESLint rules. It's mostly adjusting line lengths (100 limit) and removing whitespace. I have also tried to make blank lines between import blocks uniform.

Correct ESLint script glob

4d3a744

There was a surplus `/**/` in here, preventing these scripts from finding the files.

Re-enable additional core ESLint rules

421a310

Further correct ESLint script config and apply fixes

be06bfa

There seems to be a problem with the "zero or more folders" glob so it was missing out root files. Have got around that - we can consider regex in the future. Files here are corrected accordingly.

Re-enable multiple React/JSX related rules

e5813a5

All code fixed to conform.

Fix latest Table code according to current rules

3423bc7

This branch has also been rebased onto the latest master, meaning this should be done before further edits are made.

gavinorland force-pushed the feature/eslint-config branch from 6634f93 to 3423bc7 Compare September 3, 2018 13:26

gavinorland force-pushed the feature/eslint-config branch from 0957e5c to 15a7afa Compare September 4, 2018 08:12

gavinorland mentioned this pull request Oct 26, 2018

Feature/date input controlled govuk-react/govuk-react#420

Merged

		@@ -1,5 +1,6 @@
		import React from 'react';
		import PropTypes from 'prop-types';

Conversation

gavinorland commented Aug 17, 2018

Uh oh!

codecov bot commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevesims left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Aug 17, 2018 •

edited

Loading