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

update formatter config #11640

Merged
merged 3 commits into from
Aug 8, 2024
Merged

update formatter config #11640

merged 3 commits into from
Aug 8, 2024

Conversation

itsMapleLeaf
Copy link
Contributor

@itsMapleLeaf itsMapleLeaf commented Aug 6, 2024

Changes

Makes the following formatter changes:

  • trailingComma: "all"

    Rationale:

    • Less tedious line reordering
    • The default was changed to all in prettier v3
    • Environment support is very green (Astro potentially uses other features that are less broadly supported than this)
Reverted changes

Testing

Tests passed after running format. I got a local test failure at packages\markdown\remark\test\shiki.test.js, but it doesn't seem related.

Docs

No docs needed, internal change.

Copy link

changeset-bot bot commented Aug 6, 2024

⚠️ No Changeset found

Latest commit: 7d5cc6d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added feat: markdown Related to Markdown (scope) pkg: svelte Related to Svelte (scope) pkg: vue Related to Vue (scope) pkg: example Related to an example package (scope) pkg: react Related to React (scope) pkg: preact Related to Preact (scope) pkg: solid Related to Solid (scope) pkg: lit Related to Lit (scope) pkg: integration Related to any renderer integration (scope) pkg: create-astro Related to the `create-astro` package (scope) pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Aug 6, 2024
@itsMapleLeaf itsMapleLeaf marked this pull request as draft August 6, 2024 21:46
.git-blame-ignore-revs Outdated Show resolved Hide resolved
@Princesseuh
Copy link
Member

I'll admit that I, very subjectively of course, don't like the 80 line lengths. I use a condensed font and so it kinda result in 70% of my screen just being empty now

Going through the changes and testing 80 in the language-tools, I also (subjectively) feel like some code now takes a bigger cognitive effort to parse due to being (somewhat unnecessarily) spread over multiple lines

I could probably be convinced, just not feeling it on first look

Everything else is fine, don't care

@FredKSchott
Copy link
Member

FredKSchott commented Aug 7, 2024

trailingComma: all

Big fan of making refactoring code easier, where reordering items in an array/object/function definition leads to fewer syntax errors around missing commas where the entire file breaks. If this reduces paper-cut friction for devs on the project than I'm +1.

Double quotes result in less escapes than single quotes

I don't see this stated in the link. The link only states that prettier will choose the option that will result in the fewest escapes for that specific string, but will blindly defaulting to whatever singleQuote is set to. Can you confirm where this came from? That's a really interesting fact if true! I don't care too much about this one personally but loosely -1 if this is just entirely subjective purely on the basis of "if it ain't broke don't fix it"

printWidth: 80

Torn on this one. I personally always code with 100 columns visible and prefer the extra space, having used both 80 and 100 in different projects in the past, so this is personally a change I won't enjoy. But if others code with two columns visible or zoomed to a level where >80 columns aren't visible on their screen, I think you can make an objective accessibility argument to drop down to 80.

@FredKSchott
Copy link
Member

FredKSchott commented Aug 7, 2024

For context: we were discussing these changes originally in another Astro repo, but I set the hard hard rule that all Astro repos should use the same formatter config, copy-pasting from the astro monorepo to make it easy for team members to contribute and feel familiar across repos. So the result of this PR will also impact the prettier config we use it some/most/all other Astro repos as well.

Thanks for taking the time to kick this off in the main repo, Darius!

@florian-lefebvre
Copy link
Member

  • trailingComma: in favor
  • printWidth: against
  • singleQuote: neutral

@bluwy
Copy link
Member

bluwy commented Aug 7, 2024

  • trailingComma: in favor
  • printWidth: against. Having worked with 80 (vite) and 100 (astro). I actually quite like 100 as it doesn't break up the code as often. I have more opportunity to break up code semantically than stylistically with a larger width.
  • singleQuote: against. Agree with Fred that "if it ain't broke don't fix it"

@ematipico
Copy link
Member

  • trailing commas: yes
  • print width: neutral
  • quotes: neutral

@ascorbic
Copy link
Contributor

ascorbic commented Aug 7, 2024

  • trailingComma: neutral
  • printWidth: against
  • quotes: against

I'm neutral on trailingComma because I think there are decent arguments in favour of it that balance my strong bias against any changes to the config.

I'm against the other two because there are arguments both for and against, and we should only be changing these if there's a strong reason for doing it.

@itsMapleLeaf
Copy link
Contributor Author

itsMapleLeaf commented Aug 7, 2024

Looks like we're not strongly against trailingComma - kept.
I don't feel as strongly about printWidth than the others here - reverted.

I'll elaborate more on double quotes: anecdotally, I frequently find myself needing to change from single to double quotes due to adding an apostrophe. I'll admit I misinterpreted what Prettier was saying there 😅 thinking that it was basically saying this.

Still, reverted quotes too. I'll bet that's a me thing (even though I already count several apostrophes in this comment alone 😂). trailingComma is arguably the only important one here.


In response to some less-than-appreciated dismissive comments here and elsewhere, I'll reiterate that my motivations here are not subjective. They're mechanical, in the name of reducing friction.

  • I prefer trailing commas everywhere for easier line reordering.
  • I prefer double quotes because it requires changing quotes less often.
  • I prefer no semicolons because there's less "babysitting" when moving around code.
  • I prefer always having parenthesis on arrow functions to avoid having to add them later when adding a type annotation.

This is all small stuff, but it adds up. Microfriction is real, and I believe it affects all devs to some extent, whether they know it or not.

There are some other more radical ideas to support this, e.g. always using template strings, always using arrow block bodies, always using blocks on if/while/for/do, but Prettier doesn't automate those 🤷 and that would take my brain a bit more to adjust to, admittedly. But I wouldn't be against it if it were enforced automatically.

@itsMapleLeaf itsMapleLeaf marked this pull request as ready for review August 7, 2024 16:35
@ascorbic
Copy link
Contributor

ascorbic commented Aug 7, 2024

I'm sorry if my comments came across as dismissive. I'm not suggesting that your suggestions are arbitrary. The point I was trying to make is that even when there are objective arguments for the different options, few are as cut and dried as to avoid having arguments on both sides. My main objection is that these sort of things generate disagreements that are way out proportion to the actual impact of a change, which is why I am so biased against changing any option that's already in use.

@itsMapleLeaf
Copy link
Contributor Author

I'm sorry if my comments came across as dismissive. I'm not suggesting that your suggestions are arbitrary. The point I was trying to make is that even when there are objective arguments for the different options, few are as cut and dried as to avoid having arguments on both sides. My main objection is that these sort of things generate disagreements that are way out proportion to the actual impact of a change, which is why I am so biased against changing any option that's already in use.

I agree, and no worries. I only initially included the printWidth and quote changes because I had objective arguments for them, even if anecdotal, and I was ready to pull back if there was strong pushback nonetheless.

And, not specifically to you, but generally, I want to make clear that my motivation for trailingComma, aside from reducing microfriction, is modernizing the project. There's just no good reason to use es5. In retrospect, I think that would have made sense to put in a PR separate from other opinionated changes.

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

It's crazy that we're in 2024 and opening the "files" tab on this PR crashed my browser twice, what are we doing with our life really, is the web really the best platform to build on?

@Princesseuh
Copy link
Member

I'll take care of adding this to other repos, thank you for kicking the discussion! Apologies if any comments came as dismissive and/or rude.

@Princesseuh Princesseuh merged commit 72c7ae9 into main Aug 8, 2024
12 of 14 checks passed
@Princesseuh Princesseuh deleted the update-format-config branch August 8, 2024 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope) pkg: create-astro Related to the `create-astro` package (scope) pkg: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope) pkg: lit Related to Lit (scope) pkg: preact Related to Preact (scope) pkg: react Related to React (scope) pkg: solid Related to Solid (scope) pkg: svelte Related to Svelte (scope) pkg: vue Related to Vue (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants