absurd-sql icon indicating copy to clipboard operation
absurd-sql copied to clipboard

Prettier + ESlint + Github Actions

Open quolpr opened this issue 3 years ago • 13 comments

Before going to typescript support, I want to setup prettier and eslint first 🙂 I also added CI support(Github Actions) on any pull requests or pushes

quolpr avatar Oct 13 '21 10:10 quolpr

As for the prettier, I used the default config. But I am good if we want to adjust some of the settings

quolpr avatar Oct 13 '21 10:10 quolpr

I think the prettier config should affect the existing code as little as possible. In this case it seems like just setting singleQuote to true should do the trick.

What do you think about also setting up a pre-commit hook for eslint+prettier using husky? https://github.com/typicode/husky

rshigg avatar Oct 13 '21 11:10 rshigg

Yep, good ideas 🙂 Let me do

quolpr avatar Oct 13 '21 15:10 quolpr

Done. I also wanted to add just to the GitHub actions, but tests are failing. @jlongster could you fix them? And then I will create another PR with the GitHub actions jest support

quolpr avatar Oct 13 '21 15:10 quolpr

Thank you so much! This is great.

I'm really not a fan of pre-commit hooks. I make small commits all the time, and the majority of them are in-progress commits that fail eslint sometimes. IMHO, git commit should be as fast and simple as possible.

Let's lean on CI to enforce that eslint passes and prettier has formatted the code, and provide the same checks with a single yarn can that you can run before pushing. And provide a format command that makes prettier format all files in the codebase if something is wrongly formatted. (Contributors are expected to have prettier setup in whatever code editor they are using so that shouldn't need to be used much)

jlongster avatar Oct 13 '21 16:10 jlongster

I'm on PTO today and tomorrow so won't be able to get the tests passing until Friday or Saturday. I'm happy to also start the TS integration if I have time.

jlongster avatar Oct 13 '21 16:10 jlongster

I played around, it is pretty easy to add TS support 🙂 Once this PR merged I will make another PR with the TS setup, and we can continue work on moving the project to TS

quolpr avatar Oct 13 '21 16:10 quolpr

@jlongster also, fair points. I will make changes

quolpr avatar Oct 13 '21 16:10 quolpr

I'm really not a fan of pre-commit hooks. I make small commits all the time, and the majority of them are in-progress commits that fail eslint sometimes. IMHO, git commit should be as fast and simple as possible.

I'm also not a fan of pre-commit linting but it seems to be the standard these days. Maybe the best way to go is to just run prettier on push? I highly recommend some sort of git hook for formatting in a project like this (leaving linting to the CI is a good idea).

rshigg avatar Oct 13 '21 17:10 rshigg

TBH, as for me, I tend to avoid pre-commit hooks. We have a pre-commit hook on the project where I work with the team, but I disabled it personally. CI will show you that linting fails, plus I love fast commit too.

But on the other hand, we are using lint-staged, that will lint only changed filed, that increase the recommit time dramatically 🤔. Not sure about push cause it will lint all the files(not just that are changed), which may slow down the push time. For me, it's easier if I push and go to the next task(and then got notified that CI failed and fixed when I have time), then await the push.

TBH, it's a matter of personal preference 🤔

quolpr avatar Oct 14 '21 16:10 quolpr

I'm also not a fan of pre-commit linting but it seems to be the standard these days

I haven't heard of this standard, it must be per project. Btw, there is one caveat with pre commit hooks - what if there is a bug in code being run during commit hook or... this code gets infected by malicious code. You can loose your uncommited changes which sometimes can be many hours of work.

bartosz-k avatar Oct 21 '21 22:10 bartosz-k

and provide the same checks with a single yarn

As I understand, the single yarn is always about packages installation.

So I decided to make it as it has done in the prettier package — https://github.com/prettier/prettier/blob/7bda3a3a58392fa9469c9812fa2059bbc0840ea9/package.json#L153-L164

quolpr avatar Oct 24 '21 10:10 quolpr

@jlongster could you check? The CI is ok btw https://github.com/quolpr/absurd-sql/runs/3988469188?check_suite_focus=true

quolpr avatar Oct 24 '21 10:10 quolpr