Add tslint-plugin-prettier and apply code formatting #2810

mturley · 2019-08-30T20:18:21Z

What:

Because we realized that some important code-formatting-related TSLint rules are broken, @redallen and I decided that it would be appropriate to add Prettier to our linter, and run it on existing code.

This PR:

Adds tslint-plugin-prettier which runs Prettier as part of TSLint (currently these errors do not fail the CI checks, but eventually when other TSLint errors are resolved they probably should)
Re-adds tslint-config-prettier which was removed in Improve linting #2787, this disables TSLint rules which would conflict with Prettier
Re-adds the yarn prettier script which contributors can run to automatically format their code
Includes code changes from an initial run of yarn prettier and yarn lint:ts --fix. This cleans up all of our indentation issues and other small formatting gripes.

Hopefully this will help to make our code a little more readable and consistent going forward!

cc @redallen

Closes #2362 (for real this time)

mturley · 2019-08-30T20:21:16Z

Hmm, yarn lint:ts --fix still makes some conflicting changes. Hang on for one more commit.

patternfly-build · 2019-08-30T20:28:03Z

PatternFly-React preview: https://patternfly-react-pr-2810.surge.sh

mturley · 2019-08-30T21:03:48Z

Ok, this should be all set now. I ran yarn lint:ts --fix and it had some more changes to make, then ran yarn prettier again and it made no changes, so they don't conflict.

boaz0

🎊 🎉

mturley · 2019-09-04T17:28:42Z

Rebased on master

redallen · 2019-09-06T15:04:59Z

Let's try to do get this in post code-freeze next week after one final rebase. Things are a little hectic now.

mturley · 2019-09-06T18:03:32Z

Fair enough 🙂

mturley · 2019-09-17T19:03:44Z

(moved this comment to the bottom to track latest commits)

mturley · 2019-09-17T19:09:38Z

Hmm.. @redallen I'm having trouble parsing the errors in the CircleCI log, do you know why this would be failing CI? I didn't make any different configuration changes compared to before the rebase.

redallen · 2019-09-18T13:09:41Z

@mturley During yarn build it's something to do with Chart types:

src/components/ChartVoronoiContainer/ChartVoronoiContainer.tsx(153,6): error TS2322: Type '{ activateData?: boolean; activateLabels?: boolean; disable?: boolean; labels?: (point: any, index: number, points: any[]) => string; mouseFollowTooltips?: boolean; onActivated?: (points: any[], props: VictoryVoronoiContainerProps) => void; ... 14 more ...; theme: ChartThemeDefinitionInterface; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<VictoryVoronoiContainer> & Readonly<VictoryVoronoiContainerProps> & Readonly<{ children?: ReactNode; }>'.
  Property 'className' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<VictoryVoronoiContainer> & Readonly<VictoryVoronoiContainerProps> & Readonly<{ children?: ReactNode; }>'

Charts has been touched a lot lately, so I'd recheck the merge conflicts for that file. Worst case, checkout master's version and then relint the file.

mturley · 2019-09-18T17:35:51Z

Worst case, checkout master's version and then relint the file

Hmm, that's what I did (I cherry-picked only the config changes and relinted the whole repo). I'll take a closer look at that file, thanks!

mturley · 2019-09-18T20:56:43Z

...ges/patternfly-4/react-charts/src/components/ChartVoronoiContainer/ChartVoronoiContainer.tsx

  // Note: theme is required by voronoiContainerMixin, but @types/victory is missing a prop type
  // @ts-ignore
-  return <VictoryVoronoiContainer className={chartClassName} labelComponent={chartLabelComponent} theme={theme} {...rest} />;
+  return (


Ahh, @redallen it looks like it was this @ts-ignore not being applied anymore because the offending code was moved down a line by prettier. I'll add a prettier-ignore on the same line, and we might have to do that in the future where we need @ts-ignore on a long line.

It's weird that we can't just ts-ignore multiple lines... looks like an open issue: microsoft/TypeScript#19573

mturley · 2019-09-23T18:39:53Z

I rebased this on the latest master, and squashed it into 3 commits to make it easier to rebase in case we need to do so again:

The first commit makes the actual configuration changes, this commit should be included in a rebase.
The second commit stops prettier from making the build fail by removing ts-ignore from a line, this commit should be included in a rebase.
The last commit runs the linter and formatter on existing code, this commit should be dropped and replaced in a rebase.

mturley · 2019-09-23T20:18:17Z

@dgutride do you think it would make sense to try and merge this (after a final rebase) immediately after this week's code freeze ends, to minimize conflicts with other PRs? It doesn't change our dist, but it touches a lot of files.

redallen

LGTM

dlabrecq

Not a fan of how Prettier reformats the code. Some code is less readable, IMO -- formatted for a reason. Although, the yarn lint:ts --fix is much needed. Those changes in chart changes look ok.

mturley · 2019-09-26T14:23:14Z

@dlabrecq what formatting opinions of Prettier do you dislike? We can reconfigure it: https://prettier.io/docs/en/options.html. I'm not really concerned about how the code is formatted as long as it's formatted consistently. Messy indentation was my main gripe.

mturley · 2019-09-26T17:44:00Z

I do agree somewhat that Prettier tries too much to fit the most code into every line, but I think maybe that could be helped by lowering the print-width option. They actually recommend that you don't exceed 80 characters on that option (see the yellow box at the top of the options docs), and we have it set to 120.

I would be open to shortening this to 80 or 100 (and then obviously there would be many lines which would end up longer than that).

dlabrecq · 2019-09-26T19:39:29Z

@mturley I often use separate lines when I destructure multiple items or sometimes when importing long names, but that's just my preference to make things more readable. It's not a big deal, just not a fan. I am a big fan of running yarn lint:ts --fix, tho

mturley · 2019-09-26T20:27:24Z

I agree actually, those sound like good reasons to disagree with Prettier. We could lower the print width to make that less of an issue, and in extreme cases we could use // prettier-ignore if it's being really fussy. This PR doesn't automatically run it going forward though, so we can always run it and then change things back if we disagree with it.

mturley · 2019-09-27T17:48:42Z

@dlabrecq I'll bug you just one last time (I know you're not thinking of this as a big deal), but just for comparison's sake I made a few separate branches and ran the same process with the print-width set to 100 characters (mturley@c5fbfe4) and set to 80 characters (mturley@fc02c3b).

The lower the print-width, the more readable a lot of code gets, but files end up larger in terms of lines of code. Do you have an opinion on whether either of these would be better than the 120-character width in this PR?

dlabrecq

Looks better. Still some odd indentations in some places, but not a deal breaker.

dlabrecq · 2019-09-27T18:06:06Z

packages/patternfly-4/react-charts/src/components/ChartBullet/utils/chart-bullet-size.ts

+    ? height > defaultSize
+      ? value + (height - defaultSize) * scale
+      : value - (defaultSize - height) * scale
+    : width > defaultSize


This indentation looks much better compared to the others I noted

dlabrecq · 2019-09-27T18:07:35Z

packages/patternfly-4/react-charts/src/components/ChartPoint/ChartPoint.tsx

+    | 'triangleUp'
+    | 'dash'
+    | 'threshold'
+    | Function;


dlabrecq · 2019-09-27T18:09:19Z

packages/patternfly-4/react-charts/src/components/ChartBullet/ChartBullet.tsx

+        ? defaultPadding.top * 0.5 + (defaultPadding.bottom * 0.5 - defaultPadding.bottom) - 25
+        : title
+        ? -defaultPadding.bottom + 60
+        : -defaultPadding.bottom;


This indentation looks a little off?

It looks like Prettier will indent a nested ternary if it is nested within the true-branch (foo ? bar ? 1 : 2 : 3) but not if it is trailing / nested in the false-branch (foo ? 1 : bar ? 2 : 3). This is a bit weird but seems reasonable to me... to be honest though, maybe we should avoid nested ternary expressions unless we use parentheses to make them more clear. Maybe that would help here.

Looks like this is a subject of debate in Prettier: prettier/prettier#5814 (blog post explaining their decision to do it that way: https://prettier.io/blog/2018/11/07/1.15.0.html#flatten-else-branch-for-nested-ternaries-5039-by-suchipi-5272-by-duailibe-5333-by-ikatyang)

Huh. Adding parens around the second expression there, Prettier removes them. Maybe that's a case where // prettier-ignore would be useful if we need an exception to make that code extra clear.

I didn't realize // prettier-ignore was available. That could be quite useful for those couple of weird issues

Agreed. You can even configure a .prettierignore file to ignore entire files/directories if necessary

mturley · 2019-09-30T21:07:40Z

Rebased, if you want to re-approve @jschuler @dlabrecq @redallen

mturley · 2019-10-01T15:44:09Z

🎶 ...one more time... 🎶

patternfly-build · 2019-10-02T14:27:38Z

Your changes have been released in:

patternfly-react-extensions@2.20.9
patternfly-react-wooden-tree@2.0.5
patternfly-react@2.39.4
@patternfly/react-console@1.12.13
@patternfly/react-charts@5.0.15
@patternfly/react-core@3.112.4
@patternfly/react-docs@4.14.4
@patternfly/react-inline-edit-extension@2.11.71
demo-app-ts@3.6.11
@patternfly/react-integration@3.6.2
@patternfly/react-styled-system@3.6.40
@patternfly/react-styles@3.5.28
@patternfly/react-table@2.22.20
@patternfly/react-topology@2.8.66
@patternfly/react-virtualized-extension@1.2.56
@patternfly/react-icons@3.14.8

Thanks for your contribution! 🎉

mturley requested a review from redallen August 30, 2019 20:18

boaz0 previously approved these changes Sep 1, 2019

View reviewed changes

mturley requested a review from dgutride September 3, 2019 12:36

mturley dismissed boaz0’s stale review via 43a4fd4 September 4, 2019 17:28

mturley force-pushed the chore/prettier branch from 898b126 to 43a4fd4 Compare September 4, 2019 17:28

mturley requested review from dlabaj, jschuler and tlabaj September 5, 2019 13:07

mturley mentioned this pull request Sep 10, 2019

feat(DataToolbar): Add Data Toolbar component to experimental #2861

Merged

tlabaj assigned redallen Sep 13, 2019

mturley force-pushed the chore/prettier branch from 43a4fd4 to 51a85c0 Compare September 17, 2019 19:01

mturley commented Sep 18, 2019

View reviewed changes

mturley force-pushed the chore/prettier branch from 51a85c0 to fb5072e Compare September 18, 2019 20:57

mturley changed the title ~~Add tslint-plugin-prettier~~ Add tslint-plugin-prettier and apply code formatting Sep 23, 2019

mturley force-pushed the chore/prettier branch from fb5072e to e771fff Compare September 23, 2019 18:38

mturley force-pushed the chore/prettier branch from e771fff to 4105d86 Compare September 23, 2019 20:13

redallen previously approved these changes Sep 23, 2019

View reviewed changes

mturley dismissed redallen’s stale review via 8651a8d September 25, 2019 20:45

dlabrecq previously approved these changes Sep 25, 2019

View reviewed changes

mturley dismissed dlabrecq’s stale review via 5891669 September 26, 2019 17:44

mturley force-pushed the chore/prettier branch 2 times, most recently from 5891669 to e201d19 Compare September 26, 2019 18:00

mturley force-pushed the chore/prettier branch 2 times, most recently from b9d71eb to cccb4d7 Compare September 27, 2019 16:56

mturley mentioned this pull request Sep 27, 2019

Code style/formatting rules for TypeScript are not enforced by TSLint #2362

Closed

dlabrecq previously approved these changes Sep 27, 2019

View reviewed changes

jschuler previously approved these changes Sep 27, 2019

View reviewed changes

mturley dismissed stale reviews from jschuler and dlabrecq via b1f6012 September 30, 2019 21:06

mturley force-pushed the chore/prettier branch from cccb4d7 to b1f6012 Compare September 30, 2019 21:06

redallen previously approved these changes Sep 30, 2019

View reviewed changes

mturley added 3 commits October 1, 2019 10:56

Reconfigure TSLint and add tslint-plugin-prettier

5268f63

Add prettier-ignore where we need to use @ts-ignore on a long line

29529a5

Run yarn lint:ts --fix and yarn prettier on all existing code

4e1b48c

mturley dismissed redallen’s stale review via 4e1b48c October 1, 2019 15:44

mturley force-pushed the chore/prettier branch from b1f6012 to 4e1b48c Compare October 1, 2019 15:44

dlabrecq approved these changes Oct 1, 2019

View reviewed changes

redallen approved these changes Oct 2, 2019

View reviewed changes

redallen merged commit 0d072ea into patternfly:master Oct 2, 2019

mturley deleted the chore/prettier branch October 2, 2019 17:00

Add tslint-plugin-prettier and apply code formatting #2810

Add tslint-plugin-prettier and apply code formatting #2810

Uh oh!

Conversation

mturley commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mturley commented Aug 30, 2019

Uh oh!

patternfly-build commented Aug 30, 2019

Uh oh!

mturley commented Aug 30, 2019

Uh oh!

boaz0 left a comment

Choose a reason for hiding this comment

Uh oh!

mturley commented Sep 4, 2019

Uh oh!

redallen commented Sep 6, 2019

Uh oh!

mturley commented Sep 6, 2019

Uh oh!

mturley commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mturley commented Sep 17, 2019

Uh oh!

redallen commented Sep 18, 2019

Uh oh!

mturley commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mturley commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mturley commented Sep 23, 2019

Uh oh!

redallen left a comment

Choose a reason for hiding this comment

Uh oh!

dlabrecq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mturley commented Sep 26, 2019

Uh oh!

mturley commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlabrecq commented Sep 26, 2019

Uh oh!

mturley commented Sep 26, 2019

Uh oh!

mturley commented Sep 27, 2019

Uh oh!

dlabrecq 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!

mturley Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mturley commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mturley commented Aug 30, 2019 •

edited

Loading

mturley commented Sep 17, 2019 •

edited

Loading

mturley commented Sep 18, 2019 •

edited

Loading

mturley commented Sep 23, 2019 •

edited

Loading

dlabrecq left a comment •

edited

Loading

mturley commented Sep 26, 2019 •

edited

Loading

mturley Sep 27, 2019 •

edited

Loading

mturley commented Sep 30, 2019 •

edited

Loading