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

Wip/prettier formatting #1524

Merged
merged 22 commits into from
Feb 11, 2022

Conversation

corasaurus-hex
Copy link
Contributor

@corasaurus-hex corasaurus-hex commented Feb 11, 2022

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:

  • Read How to Contribute.
  • Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
  • Added to or updated docs in this branch, if appropriate
  • Tests
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
    • Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test.
  • Referenced the issue I am fixing/addressing in a commit message for the pull request.
    • If I am fixing the issue, I have used GitHub's fixes/closes syntax
    • [~] If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
  • Created the issue I am fixing/addressing, if it was not present.
  • Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-all)

Ping @PEZ, @bpringe

@bpringe
Copy link
Member

bpringe commented Feb 11, 2022

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?

@corasaurus-hex
Copy link
Contributor Author

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.

@corasaurus-hex
Copy link
Contributor Author

I suppose I should add the CI check since I just sang its praises.

@PEZ
Copy link
Collaborator

PEZ commented Feb 11, 2022

Here we go! 🤠

I'm afk, will have a look in a moment, but like what I read in the thread.

@PEZ
Copy link
Collaborator

PEZ commented Feb 11, 2022

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.

@corasaurus-hex
Copy link
Contributor Author

I believe this is good to go! :shipit:

@corasaurus-hex
Copy link
Contributor Author

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:

- [] Formatted all JavaScript and TypeScript code that was changed. (use the [prettier extension](https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode) or run `npm run prettier-format`)

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.

@PEZ PEZ merged commit c549d56 into BetterThanTomorrow:dev Feb 11, 2022
@PEZ
Copy link
Collaborator

PEZ commented Feb 11, 2022

Awesome. Thanks a lot for this help!

@PEZ PEZ mentioned this pull request Feb 11, 2022
@corasaurus-hex corasaurus-hex deleted the wip/prettier-formatting branch February 11, 2022 20:19
@bpringe
Copy link
Member

bpringe commented Feb 12, 2022

I second what Peter said; thanks a lot for this!

@corasaurus-hex corasaurus-hex mentioned this pull request Feb 13, 2022
16 tasks
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.

None yet

3 participants