Add prettier to pre-commit, install pre-commit in prebuild #8655

easyCZ · 2022-03-08T10:31:54Z

Description

Extends pre-commit (already installed in dev container) to include prettier (already installed in container). This will lint + fix files which do not conform to the default prettier style guide - we can tweak this if we are not happy with the defaults (discouraged).
Extends gitpod config to install the pre-commit hook into the workspace. This trigger the pre-commit run each time git commit ... is invoked. User can always run with git commit ... --no-verify to skip the hooks.

This is an incremental linter/fixer - files will be modified only when changed. This avoid a massive reformatting PR.
Where there are bigger changes, authors are encouraged to first re-format the file/s and raise a "just reformat PRs" and base their changes on top.

Related Issue(s)

This PR is complementary to some of the PRs linked in that here we wire-up pre-commit into git and run prettier, other PRs configure prettier to be prettier..

How to test

Check out branch
Make a change to a *.ts file - couple of newlines is enough
git add .
git commit -m "test" - observe linted & formatted file

Release Notes

NONE

Documentation

geropl · 2022-03-08T10:52:50Z

@easyCZ Note that we should be careful with touching files that are not 💯 under Webapp control. IMO those would also benefit from prettier, but I a separate step/process, incl. getting other teams on board.

For starts, we should explicitly restrict prettier to our components. And maybe .tsx?, so we get a chance to have a common format across teams for things like BUILD.yaml, package.json? 🤔

On the other hand, those are our files, so: let's just do the per-component filter.

easyCZ · 2022-03-08T11:08:26Z

@easyCZ Note that we should be careful with touching files that are not 💯 under Webapp control. IMO those would also benefit from prettier, but I a separate step/process, incl. getting other teams on board.

I've now scoped it to only a couple of WebApp components. If landed in this form, I'll cut a ticket to expand it for tracking.

easyCZ · 2022-03-08T11:58:32Z

.werft/jobs/build/validate-changes.ts

@@ -29,7 +29,7 @@ async function branchNameCheck(werft: Werft, config: JobConfig) {

 async function preCommitCheck(werft: Werft) {
    werft.log("pre-commit checks", "Running pre-commit hooks.")
-    const preCommitCmd = exec(`pre-commit run --all-files --show-diff-on-failure`, { slice: "pre-commit checks" });
+    const preCommitCmd = exec(`pre-commit run --show-diff-on-failure`, { slice: "pre-commit checks" });


IMO, there's limited value in running pre-commit on all files. Not only does it prevent incremental lint fix, it also takes longer.

Fine with that. I don't think we have a case of "transitive invalidation" in any of our checks,

codecov · 2022-03-08T12:02:45Z

Codecov Report

Merging #8655 (bfa75ce) into main (0f44fe9) will decrease coverage by 1.13%.
The diff coverage is n/a.

❗ Current head bfa75ce differs from pull request most recent head bdaadbc. Consider uploading reports for the commit bdaadbc to get more accurate results

@@            Coverage Diff             @@
##             main    #8655      +/-   ##
==========================================
- Coverage   12.31%   11.17%   -1.14%     
==========================================
  Files          20       18       -2     
  Lines        1161      993     -168     
==========================================
- Hits          143      111      -32     
+ Misses       1014      880     -134     
+ Partials        4        2       -2

Flag	Coverage Δ
components-gitpod-cli-app	`11.17% <ø> (ø)`
components-local-app-app-darwin-amd64	`?`
components-local-app-app-darwin-arm64	`?`
components-local-app-app-linux-amd64	`?`
components-local-app-app-linux-arm64	`?`
components-local-app-app-windows-386	`?`
components-local-app-app-windows-amd64	`?`
components-local-app-app-windows-arm64	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go

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 0f44fe9...bdaadbc. Read the comment docs.

easyCZ · 2022-03-08T13:37:31Z

Example of the current reformat is in https://github.com/gitpod-io/gitpod/pull/8664/files, which is based on top of this PR.

geropl · 2022-03-08T15:25:48Z

.gitpod.yml

@@ -40,6 +40,9 @@ tasks:
  - name: Go
    init: leeway exec --filter-type go -v -- go mod verify
    openMode: split-right
+  - name: pre-commit
+    # install pre-commit hooks into git
+    before: pre-commit install


Could it be that we already install them elsewhere? 🤔 I wondering because I think I sometimes see warnings printed on commit.

It runs on CI in
https://github.com/gitpod-io/gitpod/blob/39cf522451dfd0516116df6b089620ac60cf6489/.werft/jobs/build/validate-changes.ts#L30-L40

Outside of that, I haven't found any other usages of pre-commit install. pre-commit itself is installed in the dev Dockerfile.

I thought about in IDE, but cannot find it.

Ah, doesn't the Git initializer already install all hooks? (I think I saw it fail once because of Git hook errors like a year ago)

In my test workspace runs, it didn't install the hooks. I also didn't find any referneces to pre-commit install in the codebase so this would need to happen somewhere outside (I don't yet have context where that would be).

Also worth noting: I believe the latest Terminal (i.e. -) in tasks ends up being the one visible by default. So here, you're hiding the right Go Terminal, and showing a Terminal on top that does just pre-commit install.

Suggestions:

Maybe it's better to add pre-commit install as a before to any other task listed above

I personally really dislike the split Terminals (TypeScript on left, Go on right) and would also be very happy if we remove the split (but this is unrelated to this PR, and others might really like the split)

Thanks for raising this. I will make the requested change here to not change the experience.

geropl · 2022-03-08T15:26:14Z

.pre-commit-config.yaml

+    hooks:
+      - id: prettier
+        # Only enabled for WebApp components initially, to build consensus and incrementally onboard others
+        files: ^components\/(server|gitpod-protocol|gitpod-db|dashboard)\/.*\.ts(x?)$


easyCZ · 2022-03-08T15:41:49Z

Worth pointing out that the default prettier config uses 2 spaces indentation instead of 4 currently. See https://github.com/gitpod-io/gitpod/pull/8664/files for examples. I do not have a preference (and don't want to bikeshed) so happy to tweak that config if needed.

geropl · 2022-03-08T15:49:58Z

I do not have a preference (and don't want to bikeshed)

@easyCZ Agree 💯 with the attitude.

Just looked at the diff of some larger files, though, and it's insanely hard to read. 🙈 😆 I guess that's mostly me, and the whole point of this exercise (unification). But maybe we should double check with the other internally what they think about the specific style. Really really don't want to have these kind of discussions go astray but we also have to make sure we have an immediate benefit from this. 👍

geropl · 2022-03-09T14:08:17Z

I found some time to look into how other big TS projects do it (Theia, vscode): If they use prettier, they have a minimal .prettierrc like this:

semi: true
tabWidth: 4
singleQuote: true
printWidth: 120
trailingComma: "es5"

Especially the printWidth: 120 (default: 80) does look way saner. With this I see a direct improvement because:

it still close to our previous style to feel the same
we have the benefit of unification.

Yet I think we should:

improve the devx: currently "git commit" takes ages, and shows this at the end
maybe run prettier on-save instead of on-commit? 🤔 (could also be a follow up)
share an example and let the whole team to vote

easyCZ · 2022-03-09T16:28:50Z

I've updated the PR to include a default prettier config as suggested - you can view how the changes look like here: https://github.com/gitpod-io/gitpod/pull/8664/files

maybe run prettier on-save instead of on-commit? 🤔 (could also be a follow up)

The goal here shouldn't be to configure the IDEs, at lest not yet. We're trying to prevent inconsistent formatting from entering the main branch. As such, formatting on-commit feels appropriate, and also independent of any editor or IDE.

"git commit" takes ages

The first one takes a bit longer as it pulls the config, subsequent ones are pretty fast. It's always an option to run git commit --no-verify

We can do a follow-up to tweak how the VSCode behaves and what to do there.

easyCZ · 2022-03-10T08:54:00Z

/hold

laushinka · 2022-03-10T09:27:46Z

Thank you for this, @easyCZ!
Most of the changes look good, but I found one problem so far, which is it changes the props object from comma to semi-colon: (For the TypeScript definition I think it's not a problem because TS accepts both comma and semi-colon I think?)

easyCZ · 2022-03-10T09:30:17Z

Thank you for this, @easyCZ! Most of the changes look good, but I found one problem so far, which is it changes the props object from comma to semi-colon:

This only happens for the type definitions and is actually consistent with how type definitions are specified - for example when you're defining an interface it uses semicolons for properties. TS is lax in this and also allows commands (to make it look like a JS object notation). Again, a single style over personal preference. Here I blame TS for allowing multiple styles :)

andrew-farries

This looks good. Thanks for setting this up. Strongly agree with the sentiment of sane consistent formatting over catering for personal preferences.

laushinka · 2022-03-10T09:39:32Z

TS is lax in this

TIL!
Changes look good.

jankeromnes · 2022-03-10T09:57:21Z

.prettierrc.json

@@ -0,0 +1,7 @@
+{
+    "singleQuote": true,


Sorry to actually bike shed on this. 🙈

For JS, I'm usually team single quote, but our code base seems to use mostly double quotes. If it's okay with everyone, I think we should probably default to double quotes to make this less disruptive.

geropl · 2022-03-10T10:14:07Z

Thanks for your input everyone. Turned out relatively well, sans the bikeshedding around quotes. 🙏 😉

To shortcut the discussion here, and get this merged quickly I suggest:

we remove the "singleQuote" option: counting the mentioned personal preferences as vague trend, but more based on the proof that it's more consistent with what we currently have. The rational is that a) we still get unification, while b) removing the friction during migration
address the "Go output" hidden thing @jankeromnes mentioned here

@easyCZ Could you take care and ping me for ✔️ after that? 🙏

github-actions · 2022-03-10T13:34:14Z

⚠️ Hey reviewer! BE CAREFUL ⚠️
Review the code before opening in your Gitpod. .gitpod.yml was changed and it might be harmful.

easyCZ · 2022-03-10T13:40:17Z

@geropl This is now fixed up.

Double quotes by default
Moved pre-commit install to before - Verified the terminals show the same as on main

easyCZ · 2022-03-10T13:44:13Z

The example PR has also been updated to reflect the changes - https://github.com/gitpod-io/gitpod/pull/8664/files

geropl

console.log("Nice!");

^
| note the 4 spaces here (-:

easyCZ · 2022-03-10T13:48:39Z

/unhold

roboquat added release-note-none size/S labels Mar 8, 2022

easyCZ requested review from geropl and jankeromnes March 8, 2022 10:32

roboquat added size/M and removed size/S labels Mar 8, 2022

easyCZ force-pushed the mp/pre-commit branch from 0cc8cec to f69634f Compare March 8, 2022 11:07

mads-hartmann mentioned this pull request Mar 8, 2022

Run prettier formatting and test for formatting in builds #8066

Closed

easyCZ force-pushed the mp/pre-commit branch from f0b7b6a to 80099a8 Compare March 8, 2022 11:57

easyCZ commented Mar 8, 2022

View reviewed changes

easyCZ requested a review from a team March 8, 2022 13:12

github-actions bot added the team: webapp Issue belongs to the WebApp team label Mar 8, 2022

easyCZ force-pushed the mp/pre-commit branch from 80099a8 to 532a963 Compare March 8, 2022 13:39

geropl reviewed Mar 8, 2022

View reviewed changes

mustard-mh mentioned this pull request Mar 9, 2022

[server] remove setting sync record with limit #8644

Merged

easyCZ force-pushed the mp/pre-commit branch 2 times, most recently from bcc71fd to eba6f73 Compare March 9, 2022 19:58

roboquat added the do-not-merge/hold label Mar 10, 2022

easyCZ requested a review from a team March 10, 2022 09:11

andrew-farries previously approved these changes Mar 10, 2022

View reviewed changes

jankeromnes reviewed Mar 10, 2022

View reviewed changes

easyCZ dismissed andrew-farries’s stale review via 8f15455 March 10, 2022 10:23

easyCZ force-pushed the mp/pre-commit branch from eba6f73 to 8f15455 Compare March 10, 2022 10:23

Add prettier to pre-commit & sane default config

bdaadbc

easyCZ force-pushed the mp/pre-commit branch from 8f15455 to bdaadbc Compare March 10, 2022 13:33

easyCZ requested review from andrew-farries, geropl and jankeromnes March 10, 2022 13:40

geropl approved these changes Mar 10, 2022

View reviewed changes

roboquat removed the do-not-merge/hold label Mar 10, 2022

roboquat merged commit bc1e9fa into main Mar 10, 2022

roboquat deleted the mp/pre-commit branch March 10, 2022 13:49

easyCZ mentioned this pull request Mar 10, 2022

build: add Prettier support and configurations #8220

Closed

trumbitta mentioned this pull request Mar 10, 2022

[dashboard] Lint warnings #3841

Closed

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Mar 15, 2022

trumbitta mentioned this pull request Apr 20, 2022

Pre-install Prettier and ESLint VSCode extensions #9269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prettier to pre-commit, install pre-commit in prebuild #8655

Add prettier to pre-commit, install pre-commit in prebuild #8655

easyCZ commented Mar 8, 2022 •

edited

Loading

geropl commented Mar 8, 2022 •

edited

Loading

easyCZ commented Mar 8, 2022 •

edited by werft-gitpod-dev-com bot

Loading

easyCZ Mar 8, 2022 •

edited

Loading

geropl Mar 8, 2022

codecov bot commented Mar 8, 2022 •

edited

Loading

easyCZ commented Mar 8, 2022 •

edited

Loading

geropl Mar 8, 2022

easyCZ Mar 8, 2022

easyCZ Mar 8, 2022

geropl Mar 8, 2022

jankeromnes Mar 10, 2022

easyCZ Mar 10, 2022

jankeromnes Mar 10, 2022

easyCZ Mar 10, 2022

geropl Mar 8, 2022

easyCZ commented Mar 8, 2022 •

edited

Loading

geropl commented Mar 8, 2022

geropl commented Mar 9, 2022 •

edited

Loading

easyCZ commented Mar 9, 2022 •

edited

Loading

easyCZ commented Mar 10, 2022

laushinka commented Mar 10, 2022 •

edited

Loading

easyCZ commented Mar 10, 2022

andrew-farries left a comment

laushinka commented Mar 10, 2022 •

edited

Loading

jankeromnes Mar 10, 2022 •

edited

Loading

geropl commented Mar 10, 2022 •

edited

Loading

github-actions bot commented Mar 10, 2022

easyCZ commented Mar 10, 2022

easyCZ commented Mar 10, 2022

geropl left a comment

easyCZ commented Mar 10, 2022

Add prettier to pre-commit, install pre-commit in prebuild #8655

Add prettier to pre-commit, install pre-commit in prebuild #8655

Conversation

easyCZ commented Mar 8, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

geropl commented Mar 8, 2022 • edited Loading

easyCZ commented Mar 8, 2022 • edited by werft-gitpod-dev-com bot Loading

easyCZ Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 8, 2022 • edited Loading

Codecov Report

easyCZ commented Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easyCZ commented Mar 8, 2022 • edited Loading

geropl commented Mar 8, 2022

geropl commented Mar 9, 2022 • edited Loading

easyCZ commented Mar 9, 2022 • edited Loading

easyCZ commented Mar 10, 2022

laushinka commented Mar 10, 2022 • edited Loading

easyCZ commented Mar 10, 2022

andrew-farries left a comment

Choose a reason for hiding this comment

laushinka commented Mar 10, 2022 • edited Loading

jankeromnes Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

geropl commented Mar 10, 2022 • edited Loading

github-actions bot commented Mar 10, 2022

easyCZ commented Mar 10, 2022

easyCZ commented Mar 10, 2022

geropl left a comment

Choose a reason for hiding this comment

easyCZ commented Mar 10, 2022

easyCZ commented Mar 8, 2022 •

edited

Loading

geropl commented Mar 8, 2022 •

edited

Loading

easyCZ commented Mar 8, 2022 •

edited by werft-gitpod-dev-com bot

Loading

easyCZ Mar 8, 2022 •

edited

Loading

codecov bot commented Mar 8, 2022 •

edited

Loading

easyCZ commented Mar 8, 2022 •

edited

Loading

easyCZ commented Mar 8, 2022 •

edited

Loading

geropl commented Mar 9, 2022 •

edited

Loading

easyCZ commented Mar 9, 2022 •

edited

Loading

laushinka commented Mar 10, 2022 •

edited

Loading

laushinka commented Mar 10, 2022 •

edited

Loading

jankeromnes Mar 10, 2022 •

edited

Loading

geropl commented Mar 10, 2022 •

edited

Loading