-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Check formatting in CI #2736
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
Check formatting in CI #2736
Conversation
f988432
to
ab442f1
Compare
45a9b86
to
9d5ef15
Compare
c627bb5
to
00c0214
Compare
00c0214
to
942af6e
Compare
942af6e
to
c2934e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The React
import is only necessary for old versions of the JSX transform. We use Vite so it should not be necessary, that's why prettier is automatically removing it.
@sodic wanted to fight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extracted the common steps of setting up Haskell + the Cabal cache to a common action with the intention of using it later for waspc-ci.yaml
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff
862a28e
to
81504af
Compare
The mods are asleep, merge Yeah, I like trailing commas. I'm guessing we all know the arguments for this rule so I won't go into details. But I won't die defending my position if everybody is against it. Does someone here not like them? Why not, I'm curious? |
I'm a +1 for trailing commas everywhere |
import { FormEvent, FormEventHandler } from 'react' | ||
import { type AuthUser as User } from 'wasp/auth' | ||
import { logout } from 'wasp/client/auth' | ||
import { | ||
createTask, | ||
updateTask, | ||
deleteTasks, | ||
useQuery, | ||
getTasks, | ||
} from "wasp/client/operations"; | ||
import React, { FormEventHandler, FormEvent } from "react"; | ||
import waspLogo from "./waspLogo.png"; | ||
updateTask, | ||
useQuery, | ||
} from 'wasp/client/operations' | ||
import { type Task } from 'wasp/entities' | ||
import waspLogo from './waspLogo.png' | ||
|
||
import "./Main.css"; | ||
import './Main.css' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointing out that imports get re-ordered and pruned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: from my experience won't work on some mustache template files. So we will stave old imports there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I've completely disabled any formatting in the templates, not worth the trouble
git config --global core.autocrlf input | ||
git config --global core.eol lf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should team members do something like this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmm yes, if they were on Windows, it's kind of the first step to setup Git there. This is just copied from wasp-ci.yaml
for good measure, but I'm not sure how needed it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it as this only runs on ubuntu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some questions. The rules look good to me, but I'd like to keep double quote for JSX, I'm more used to it and I think it's more common?
One thing to add, we have all the actions team members might need to do in the run
script: https://github.com/wasp-lang/wasp/blob/main/waspc/run#L114-L117 Let's add the prettier commands there.
Quick resume of my opinions:
EDIT because Franjo mentioned these topics:
@cprecioso I am curious about the experience for us, since I am not that knowledgeable about these configs -> how much setup is needed on our side? I am guessing not much, config is shared, and correct version of prettier will be installed per each project because it is specific in package.json, right? There is no need to use globally installer version of prettier I hope? |
Nope,
Just
I didn't change anything in ormolu's setup. AFAICT we pin its version through |
Ok sounds great! One thing we don't do for |
My preferences: trailing commas always I'm okay with whatever has the highest number of votes winning. |
81504af
to
e5aed94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes since last version:
- Removed unnecessary git config
- Set up scripts in
./run
- We're now just using the default
prettier
config - Removed tailwind plugin since @FranjoMindek said it doesn't work as well as it should
- Added
// prettier-ignore
annotations to the prebuilt Posthog scripts (piggy.js and posthog.js) - We had an empty Dockerfile (which is not valid), so I added a
RUN echo
instruction so that we can format it.
e5aed94
to
3fd700d
Compare
My approve still stands 👍 |
.prettierrc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we maybe want to list out all the defaults we expect to use, what if the Prettier default changes in the meantime. We made some explicit choices, it's not too bad to encode them even if they match the defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I've not seen that in a lot of places I think. I think it's not really needed.
We will have opportunities to change this if we ever need to, since the prettier dependencies are pinned (no ^version
) as they recommend:
https://github.com/wasp-lang/wasp/blob/3fd700d56f86a383154ecd3e6aced2e1a5f3323f/package.json#L8-L11
And any changes they make will affect us anyway, since they have explicitly communicated that they are against options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can get behind their philosophy 👍 I guess our options are "whatever Prettier does".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I think I misunderstood before. I agree with you that it is a good idea to add the explicit choices. Let me cook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, will merge when CI passes
3fd700d
to
1be45ce
Compare
…sal-code-style-formatting-and-linting-rules
1be45ce
to
055861b
Compare
.editorconfig
to the repo.prettierrc
and.prettierignore
formatting to the repoprettier-plugin-organize-imports
: to deterministically order importsprettier-plugin-pkg
: deterministically orders package.jsonprettier-plugin-sh
: adds support for shell scriptsprettier-plugin-tailwindcss
: formats tailwind class stringsnpx prettier --write .
)Tip
Remove the last "Format" commit from the review pane so that you can easily review the changes. The last commit is just
npx prettier --write .
.