-
-
Notifications
You must be signed in to change notification settings - Fork 223
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
Wip/prettier formatting #1524
Wip/prettier formatting #1524
Conversation
Thanks! Maybe we could even use a git pre-commit hook to run that formatting script instead of requiring the person runs the formatter themselves? CC @PEZ I'd prefer a tabWidth of 2 to prevent code from starting so far from the start of the line in places where there's pretty nested code, but maybe this is not the usual tabWidth most people are used to? |
A pre-commit hook would work, for sure! There are some edge cases to cover that are tricky but it's doable. Either way, though, we'd want to add a format check to the CI checks. That way no badly formatted code slips in somehow. I usually just forgo the pre-commit hook and just keep the CI check. The pre-commit hook makes committing feel so slow and you can just use the vscode prettier plugin to format on save as you go. If people don't want that convenience (and omg why not?!) then it's easy enough for them to run the formatter task before the PR gets accepted. I prefer tab width of 2, myself! But making it 2 would have meant changing every indented line of js and ts in the project 😬. I didn't think that PR would get accepted and so I was just trying to mostly match the current formatting but with some little tweaks that improve readability (she says, having opened a +10k line PR 😅). I'm more than happy to change it to 2 and reformat-all-the-things. Git blame will be a little weird for a while, though, since I'd be the last person to change almost every line. |
I suppose I should add the CI check since I just sang its praises. |
Here we go! 🤠 I'm afk, will have a look in a moment, but like what I read in the thread. |
We can start with tabWidth 4, I think. If I read the config right it is trying to codify what looks to be some sort of srandard in the current code base. Is this good to go now, @corasaurus-hex ? Looks like so to me. This is going to hurt a bit for some pending PRs so I think it's better to rip the plaster asap. 😄 We should update the wiki and add some recommendations there to ease the friction. I think there even is a page about code format standard that probably does not need to say so much any longer. |
I believe this is good to go! |
Oh, yep, here's the page that needs some adjustments: https://github.com/BetterThanTomorrow/calva/wiki/Coding-Style I should be able to get to this tomorrow. In the meantime I've added a checkbox to the end of the pull request template that looks like:
And for anyone who finds this PR, the nicest setup (imo) is to install the prettier extension for vscode. You can set it to format on save which means you don't need to format your code perfectly yourself, you need only save and let the format get fixed automatically and immediately. |
Awesome. Thanks a lot for this help! |
I second what Peter said; thanks a lot for this! |
What has Changed?
Consistently format js and ts files using prettier.
I was talking to @PEZ today and he lamented that PRs often have inconsistent whitespace changes. One great way to address this issue with javascript and typescript is to use prettier. If the user adds this extension then the editor can be set to automatically format the file correctly. And even if the user doesn't have that extension you can ask them to run
npm run prettier-all
to fix their PR formatting before submitting PRs.It's important to note that formatting with prettier does not change how the code functions, it's merely to standardize the formatting.
Fixes #1523
My Calva PR Checklist
I have:
dev
branch. (Or have specific reasons to target some other branch.)published
. (Sorry for the nagging.)[Unreleased]
entry inCHANGELOG.md
, linking the issue(s) that the PR is addressing.ci/circleci: build
test.npm run prettier-all
)Ping @PEZ, @bpringe