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

Force local linting #378

Merged
merged 3 commits into from
Nov 25, 2024
Merged

Conversation

mrsimpson
Copy link
Collaborator

Motivation

Linting as in #367 is essential for concise PRs.
Linting should be enforced prior to merging, latest in CI.

However, it's a bad contributor experience and a waste of resources if a contributor pushes his changes, considers things done and only a pipeline fail tells him that his IDE forgot to prettify for him/her ;)

What's in this PR

This PR adds a pre-commit-hook which lints the code and fixes easy fixes (such as blanks or semicolons) on the contributor's machine. As the name suggests, this process runs prior to completing the commit, meaning the developer can't commit with linting issues.

Considerations

  • This PS is based upon Linting #367 and should be merged 🔜 after this PR
  • Inexperienced developers may not understand that the linting happens pre-commit. I added a section to the contributions-guidelines to make this clear. Those who don't read this, well ...
  • Once there's a CI/CD in place, we could relax the check in various ways (e. g. that it only lints but doesn't fail), but I'd not recommend to do that now.

@mrsimpson mrsimpson mentioned this pull request Nov 22, 2024
@mrsimpson
Copy link
Collaborator Author

Give me a second with the actual merge, I figured that lint:fix might not be the best experience: If there are linting errors and the pre-commit fixes them, those changes will stay in the staging area but will not be automatically commited.
Thus, a subsequent pre-commit check will succeed without adding those to the commit.

Thus, I prefer some script which just checks and informs the developer about what he's got to do. I'll add that in a few

@mrsimpson mrsimpson marked this pull request as draft November 23, 2024 08:53
@mrsimpson mrsimpson marked this pull request as ready for review November 23, 2024 09:08
@mrsimpson
Copy link
Collaborator Author

mrsimpson commented Nov 23, 2024

I added a small shell script which informs the developer what's going on.
Here's how it looks like

image

I do think this is better DX than linted files that are stuck in the staging area 😬

Edit: Added a space after the ❌ which looked ugly.

I figured that lint:fix might not be the best experience: If there are linting errors and the pre-commit fixes them, those changes will stay in the staging area but will not be automatically commited.
Thus, a subsequent pre-commit check would succeed without these changes included.
@wonderwhy-er
Copy link
Collaborator

wonderwhy-er commented Nov 25, 2024

Soo. I did checkout, I use webstorm and it seems it ignores it and commits without it :D
I kinda view this as connivance.
In the end its repo rules that do not allow to merge that really enforce this.
I don't mind merging if it makes contributors life better.

@wonderwhy-er
Copy link
Collaborator

Looks like two people like it.
I guess let's go.

@wonderwhy-er wonderwhy-er merged commit f0a668b into stackblitz-labs:main Nov 25, 2024
1 check passed
@mrsimpson mrsimpson deleted the force-local-linting branch November 25, 2024 22:24
@mrsimpson
Copy link
Collaborator Author

Soo. I did checkout, I use webstorm and it seems it ignores it and commits without it 🫥

This is not a feature of the IDE, but if git itself. Even webstorm can't ignore it 😬

The hook is installed on first pnpm i . Did you perform it?

@wonderwhy-er
Copy link
Collaborator

In the end what I feared happend for me
This failed for me in WebStorm...

In terminal its ok
image

When I run IDE Git related things I get this
Screenshot 2024-12-02 at 20 02 23

I just spent 15 minutes trying to make it work and I give up lol...
If you have ideas or desire to help I can give it a try....

But until then I at least disable precomit hook for myself.
And I am wondering how many may have such problem as well...
I am kinda not a fan of precomit hooks due to this additional dependency that fails in various situations.

@mrsimpson
Copy link
Collaborator Author

mrsimpson commented Dec 2, 2024

@wonderwhy-er it was not the typecheck that failed, but the subsequent linting.
If you run pnpm lint on the command-line, you'd see the same error.
Also, if you committed on the command-line, you'd get the same error.

The error also says "❌ Linting failed".

How could we improve verboseness?

But as the error also says: all of them should be fixable. Simply run pnpm lint:fix

@mrsimpson
Copy link
Collaborator Author

edit: @wonderwhy-er added a fix to the error message to better help you ;)

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

Successfully merging this pull request may close these issues.

4 participants