Skip to content

Add formatting rules #2219

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

Closed
coolsoftwaretyler opened this issue Nov 10, 2024 · 5 comments · Fixed by #2226
Closed

Add formatting rules #2219

coolsoftwaretyler opened this issue Nov 10, 2024 · 5 comments · Fixed by #2226
Assignees

Comments

@coolsoftwaretyler
Copy link
Collaborator

Just a small issue to capture this comment: we need to add auto-formatting to this repository so that all PRs are consistent and to reduce some of the noisy formatting diffs we tend to get.

@coolsoftwaretyler
Copy link
Collaborator Author

Here's the plan:

  1. Let's merge this PR (I am still reviewing but it looks good so far)
  2. I will actually release the next major version, including this
  3. Once we've released that version, let's do a big formatting PR. I have no strong opinions about formatting other than it should be automatic, checked before merge, and consistent. So we can just use whatever settings you have if you want the PR, or I can put the PR together and use whatever settings I have lol.
  4. We can merge that PR right behind the major version, which should keep future patches/minors/majors easy to check berfore/after the big formatting PR. Basically making it easy to see what happened between v6 and v7, and then changes inside of 7 itself.

@thegedge - I can handle this to coordinate with the major version release, unless you want to get that sweet commit credit.

@coolsoftwaretyler coolsoftwaretyler self-assigned this Nov 10, 2024
@thegedge
Copy link
Collaborator

thegedge commented Nov 10, 2024

I have no strong opinions about formatting other than it should be automatic, checked before merge, and consistent.

💯 I have no strong opinions either (I have my preferences, of course!), but agreed that it just needs to be automatic to not waste anyone's time.

Prettier is of course the go-to tool these days, but I know not everyone is behind its prescriptiveness. I'm not as deep into the ecosystem, so perhaps you have some other suggestions, @coolsoftwaretyler?

If not, I say we use prettier and call it a day. My recommendations:

  1. I'd be for switching to the user of semi-colons. Prettier will add them for us, and we avoid the dreaded (or at least I think it's dreaded) ;(a as xyz).vaue = expression syntax (used to avoid accidentally making a function call).
  2. I'm okay with a print width > 80. I'd recommend 100 as a compromise, but a-o-k if we want to stick with the default of 80.

[EDIT]
My bad for accidentally closing. I think I accidentally hit some keyboard shortcut 🙈

@thegedge thegedge reopened this Nov 10, 2024
@thegedge
Copy link
Collaborator

Actually, ignore my recommendations. I think we should respect the choices of the organization, which is in disagreement with (1) but not (2).

Let's just copy the prettier settings from https://github.com/mobxjs/mobx

@coolsoftwaretyler
Copy link
Collaborator Author

Great find, I agree!

@coolsoftwaretyler
Copy link
Collaborator Author

Alrighty, v7.0.0 is out, so I'll tackle this sometime soon.

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 a pull request may close this issue.

2 participants