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

Add prettier support #447

Merged
merged 3 commits into from
May 17, 2024
Merged

Add prettier support #447

merged 3 commits into from
May 17, 2024

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented May 16, 2024

This PR contains 4 commits:

  1. 9ca6167 adding prettier support to @npmcli/template-oss
  2. d020c1d enabling prettier on this repo by setting templateOSS.prettier: true
  3. 685dd64 running prettier to format this repo
  4. 770a10c add a .git-blame-ignore-revs file referencing the commit from item 3 Dropped from this PR since SHAs won't match after rebase

Once this lands, enabling prettier on another repo will involve updating to the new version of @npmcli/template-oss, and then running steps 2-4 which should be handled by a new script in https://github.com/npm/stafftools (or another tool).


The most important part of item 1 is the implementation of the lint and lintfix run-scripts. They now look like this:

{
    "lint": "npm run eslint && npm run prettier -- --check",
    "lintfix": "npm run eslint -- --fix && npm run prettier -- --write"
}

The goal here was to have the same lint and lintfix scripts we currently have also be responsible for running prettier. The tradeoff is that if linting fails, then formatting via prettier doesn't run. I think this is ok since it allows for a single script to handle both eslint and prettier.

We also explored using a combination of postlint and postlintfix for this, but then prettier --check (via lint->postlint) would get run before prettier --write which would defeat the purpose of prettier --write.


Other notable items are:

  • Uses @github/prettier-config except for changing bracketSpacing: true to match how we previously had eslint setup. This also sets arrowParens: "avoid" which seems like something we've never standardized on previously, so we opted to leave it as configured in the @github config.
  • Uses eslint-config-prettier to overwrite all the eslint formatting rules we use. Alternatively we could remove them all from @npmcli/eslint-config on a semver major of that. I think it's easier to get more things on prettier before a decision on that is made.

@lukekarrys lukekarrys changed the title lk/prettier Add prettier support May 16, 2024
@lukekarrys lukekarrys requested a review from jumoel May 16, 2024 00:28
test/release/release-please.js Dismissed Show dismissed Hide dismissed
@wraithgar
Copy link
Member

This may warrant .git-blame-ignore-revs

@lukekarrys lukekarrys force-pushed the lk/prettier branch 2 times, most recently from 01438fb to ca038fe Compare May 16, 2024 00:47
@lukekarrys lukekarrys marked this pull request as ready for review May 16, 2024 00:51
@lukekarrys lukekarrys requested a review from a team as a code owner May 16, 2024 00:51
lukekarrys and others added 3 commits May 15, 2024 18:34
Co-authored-by: Julian Møller Ellehauge <git@jumoel.com>
Co-authored-by: Julian Møller Ellehauge <git@jumoel.com>
jumoel
jumoel previously approved these changes May 16, 2024
Copy link
Contributor

@jumoel jumoel left a comment

Choose a reason for hiding this comment

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

Nice 💅

Ensure that this isn't squash-merged, or the rev in .git-blame-ignore-revs will be wrong.

@wraithgar
Copy link
Member

Will rebase-merge preserve the commit sha?

@lukekarrys
Copy link
Contributor Author

lukekarrys commented May 16, 2024

I checked on npm/cli for a recent multiple commit PR that was rebased and even a rebase merge will also change the sha.

For this PR I will probably land it, temporarily change branch protections, and then force push a new commit to main referencing the correct commit.

For other repos I think the prettier application and git-blame-ignore-revs creation will need to be multiple PRs.

@wraithgar
Copy link
Member

For other repos I think the prettier application and git-blame-ignore-revs creation will need to be multiple PRs.

Let's practice it here then. Don't bother disabling the branch protections. Search for past "lint everything" commits to this repo, including the one from this PR, and make a new PR adding them. That way we can live the process we'll be using going forward.

@lukekarrys
Copy link
Contributor Author

@wraithgar Good call. Just dropped the .git-blame-ignore-revs commit from the PR and will make a followup adding the SHA for the prettier formatting commit as well as any other lint commits I can find.

@wraithgar
Copy link
Member

It seems appropriate that @jumoel or @reggi approve this.

@lukekarrys lukekarrys merged commit b35bca5 into main May 17, 2024
37 checks passed
@lukekarrys lukekarrys deleted the lk/prettier branch May 17, 2024 14:01
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