Skip to content

Conversation

cprecioso
Copy link
Member

@cprecioso cprecioso commented May 12, 2025

  1. Adds a root .editorconfig to the repo
    • Just some basic options (will be taken into account for prettier)
  2. Adds a root .prettierrc and .prettierignore formatting to the repo
    • Default options, plus the editorconfig, plus:
      {
        // From the assorted configs already in our repo
      	"semi": false,
      	"singleQuote": true,
      	"trailingComma": "es5",
      	"jsxSingleQuote": true,
        // Specifically requested by @martinsos
      	"experimentalOperatorPosition": "start"
      }
    • Plugins:
      • prettier-plugin-organize-imports: to deterministically order imports
        • This is the same action that you can run in VS Code ("Organize imports")
        • It still respects clusters of imports broken up by double newlines
      • prettier-plugin-pkg: deterministically orders package.json
      • prettier-plugin-sh: adds support for shell scripts
      • prettier-plugin-tailwindcss: formats tailwind class strings
    • Ignored:
      • Templates: prettier gets confused with the mustaches
      • Markdown: prettier is only compatible with MDXv1 for now, not v3
  3. Removes all the other prettier configurations throughout the repo, we don't need them anymore
  4. Adds a CI step that runs ormolu and prettier and fails if any file is not correctly formatted
  5. Formatted everything prettier complained about (ormolu was already passing) (npx 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 ..

@cprecioso cprecioso linked an issue May 12, 2025 that may be closed by this pull request
2 tasks
@cprecioso cprecioso force-pushed the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch 5 times, most recently from f988432 to ab442f1 Compare May 12, 2025 10:26
@cprecioso cprecioso changed the title Automatic formatting WIP: Check formatting in CI May 12, 2025
@cprecioso cprecioso force-pushed the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch 4 times, most recently from 45a9b86 to 9d5ef15 Compare May 12, 2025 11:20
@cprecioso cprecioso force-pushed the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch 7 times, most recently from c627bb5 to 00c0214 Compare May 12, 2025 14:27
@cprecioso cprecioso force-pushed the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch from 00c0214 to 942af6e Compare May 12, 2025 14:34
@cprecioso cprecioso changed the title WIP: Check formatting in CI Check formatting in CI May 12, 2025
@cprecioso cprecioso force-pushed the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch from 942af6e to c2934e2 Compare May 12, 2025 14:45
Copy link
Member Author

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.

@cprecioso cprecioso marked this pull request as ready for review May 12, 2025 14:59
@FranjoMindek
Copy link
Contributor

@sodic wanted to fight trailingComma es5 vs all setting, but he is away currently.

Copy link
Member Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff

@cprecioso cprecioso force-pushed the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch from 862a28e to 81504af Compare May 13, 2025 09:56
@sodic
Copy link
Contributor

sodic commented May 15, 2025

@sodic wanted to fight trailingComma es5 vs all setting, but he is away currently.

The mods are asleep, merge trailingComma: false :)

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?

@cprecioso
Copy link
Member Author

I'm a +1 for trailing commas everywhere

Comment on lines 1 to 15
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'

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

@cprecioso cprecioso May 16, 2025

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

Comment on lines 18 to 19
git config --global core.autocrlf input
git config --global core.eol lf
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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

@cprecioso cprecioso requested review from Martinsos and sodic May 16, 2025 09:10
Copy link
Contributor

@infomiho infomiho left a 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.

@Martinsos
Copy link
Member

Martinsos commented May 16, 2025

Quick resume of my opinions:

  • I love having semicolons at the end, had my no-semicolon phase for years but am now back in semicolon camp. That said, if majority doesn't want semicolons, I am ok with that.
  • I don't care about trailing colons.
  • I prefer double quotes over single quotes, but it seems others also do so great.
  • experimentalOperatorPosition -> I don't have strong opinion, but I do like it being in front a bit better due to diffing and reading. But I don't care that much.

EDIT because Franjo mentioned these topics:

  • I also like 100 as width.
  • I strongly care about 2 spaces as tab width (but I suspect everybody wants the same).

@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?
What about ormolu, I think for it it is a bit trickier how to ensure we all have the same version -> have you been playing with that? If not, we can also look together into that.

@cprecioso
Copy link
Member Author

cprecioso commented May 16, 2025

@Martinsos

There is no need to use globally installer version of prettier I hope?

Nope, npm install on the local project is enough.

how much setup is needed on our side?

Just npm install and npm run format. (I'l look into getting it into the ./run script)
Best experience IMO is by configuring format on save in your editor (VSCode, emacs). The editor should pick up the installed prettier version.

What about ormolu, I think for it it is a bit trickier how to ensure we all have the same version

I didn't change anything in ormolu's setup. AFAICT we pin its version through dev-tool.project, and that's what ./run ormolu:format uses.

@Martinsos
Copy link
Member

Martinsos commented May 16, 2025

@Martinsos

There is no need to use globally installer version of prettier I hope?

Nope, npm install on the local project is enough.

how much setup is needed on our side?

Just npm install and npm run format. (I'l look into getting it into the ./run script) Best experience IMO is by configuring format on save in your editor (VSCode, emacs). The editor should pick up the installed prettier version.

What about ormolu, I think for it it is a bit trickier how to ensure we all have the same version

I didn't change anything in ormolu's setup. AFAICT we pin its version through dev-tool.project, and that's what ./run ormolu:format uses.

Ok sounds great! One thing we don't do for ormolu right now is set up our editors to use the locally installad version, which is installed with ./run ormolu:format (or some other similar commands, can't remember), we all mostly instead use globally installed ormolu. But that is more on each of us individually to set up I guess, I think @FranjoMindek already does it correctly. The main reason why we didn't do it is because the way it is installed locally is a bit unortodox, it is not as smooth as with installing prettier locally, but it does work. Ok, I will leave this comment here for posterity, although I said everything really.

@FranjoMindek
Copy link
Contributor

FranjoMindek commented May 16, 2025

My preferences:

trailing commas always
double quotes on jsx
semicolons
100 width (but 80 is okay)
2 spaces tab width

I'm okay with whatever has the highest number of votes winning.

@cprecioso cprecioso force-pushed the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch from 81504af to e5aed94 Compare May 22, 2025 13:15
Copy link
Member Author

@cprecioso cprecioso left a 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.

@cprecioso cprecioso force-pushed the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch from e5aed94 to 3fd700d Compare May 22, 2025 13:35
@infomiho
Copy link
Contributor

My approve still stands 👍

.prettierrc Outdated
Copy link
Contributor

@infomiho infomiho May 22, 2025

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.

Copy link
Member Author

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.

Copy link
Contributor

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".

Copy link
Member Author

@cprecioso cprecioso May 22, 2025

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.

Copy link
Member Author

@cprecioso cprecioso May 22, 2025

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

@cprecioso cprecioso force-pushed the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch from 3fd700d to 1be45ce Compare May 22, 2025 14:24
@cprecioso cprecioso force-pushed the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch from 1be45ce to 055861b Compare May 22, 2025 14:29
@cprecioso cprecioso merged commit bd32a96 into main May 22, 2025
7 checks passed
@cprecioso cprecioso deleted the cprecioso/2583-universal-code-style-formatting-and-linting-rules branch May 22, 2025 14:44
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.

Universal code style, formatting and linting rules

5 participants