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

Feat/add formatting config #2226

Merged
merged 3 commits into from
Dec 2, 2024
Merged

Conversation

coolsoftwaretyler
Copy link
Collaborator

@coolsoftwaretyler coolsoftwaretyler commented Nov 27, 2024

What does this PR do and why?

Closes #2219.

This changes our Prettier config to match the config for MobX.

It also runs bun prettier:write against the whole codebase, so there's a huge diff here.

We actually have Husky set up to do this as a pre-commit hook already. I think the reason we've had a bunch of diffs is because some of the source files hadn't been given the correct formatting at some point, so as we've made changes throughout the codebase, the Husky hook has formatted them for us.

I've also added a step to run prettier:check in CI, so if people skip the hook, we'll see the problem in PRs.

Steps to validate locally

You should have [typescript]editor.formatOnSave: true in .vscode/settings.json. Turn that to false, then make a change to a file that breaks the Prettier config.

Save the file, add it, and commit it. Husky should clean up your code.

bun test should pass, and bun run build should pass and work.

CI should pass on this branch, including the new check-prettier job.

@coolsoftwaretyler coolsoftwaretyler force-pushed the feat/add-formatting-config branch 2 times, most recently from a7c3d63 to 1d0fa8e Compare November 27, 2024 01:30
@coolsoftwaretyler
Copy link
Collaborator Author

Hey @thegedge - whaddya think? I think we've actually had this in place, but the starting point for all our files was off, so it looked like it was broken.

This adopts MobX's style, and also sets up a CI job to check Prettier on PRs.

I notice we're still on TSLint which has been deprecated for five years lol. I'm gonna open a separate issue to move to ESLint and its TypeScript tooling.

@thegedge
Copy link
Collaborator

🙌 🎉

I checked out this branch, saved a file, and it still had changes. Seems like a couple of things:

  1. I have things set up, globally, to organize imports. Not sure if we're deviating from mobx here, but I really like having imports sorted and organized (and it removes unused ones too!). What do you think of including https://www.npmjs.com/package/prettier-plugin-organize-imports? Otherwise, I can just turn this off for MST :)
  2. Ternaries are formatting differently for me (see screenshot, src/core/mst-operations.ts)

CleanShot 2024-11-27 at 16 27 18@2x

@coolsoftwaretyler
Copy link
Collaborator Author

Hey @thegedge - I like that import organizing rule as well. Since you're one of the top contributors these days, let's slightly deviate from MobX on this one so you don't need to make any changes yourself. I think that's pretty fair.

I also like removing unused imports automatically. That's a big selling point for me.

I can't reproduce your ternary issue. Any chance that's some other global config overriding the project? This is what I see with format-on-save:

Screen.Recording.2024-11-30.at.7.26.59.PM.mov

And if I turn off format-on-save and run yarn prettier:write:

Screen.Recording.2024-11-30.at.7.29.21.PM.mov

@coolsoftwaretyler coolsoftwaretyler force-pushed the feat/add-formatting-config branch from 905cf68 to 54e946e Compare December 1, 2024 02:31
@coolsoftwaretyler
Copy link
Collaborator Author

Oh dang, there's some kind of funky import order dependency here that's failing a bunch of tests. I might be inclined to actually drop that commit and just go with the MobX defaults, rather than wrestle with these weird types for now.

If we cut a new branch for bigger rewrite/refactoring, we could turn that plugin on for that endeavor and try to avoid whatever import magic is going on right now.

@coolsoftwaretyler coolsoftwaretyler force-pushed the feat/add-formatting-config branch from 1d0fa8e to 13827e6 Compare December 1, 2024 02:41
@alyssadicarlo alyssadicarlo mentioned this pull request Dec 1, 2024
@thegedge
Copy link
Collaborator

thegedge commented Dec 1, 2024

I can't reproduce your ternary issue. Any chance that's some other global config overriding the project? This is what I see with format-on-save:

Maybe! I'll double check sometime this week, but feel free to not wait for me. Happy to ship as-is!

@coolsoftwaretyler
Copy link
Collaborator Author

Sweet. Shippin it!

@coolsoftwaretyler coolsoftwaretyler merged commit 6c2cad9 into master Dec 2, 2024
1 check passed
@coolsoftwaretyler coolsoftwaretyler deleted the feat/add-formatting-config branch December 2, 2024 01:10
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.

Add formatting rules
2 participants