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 lint hook to only lint files that have been changed for the current branch #1333

Open
mcmire opened this issue May 5, 2023 · 1 comment · May be fixed by #4261
Open

Update lint hook to only lint files that have been changed for the current branch #1333

mcmire opened this issue May 5, 2023 · 1 comment · May be fixed by #4261
Assignees

Comments

@mcmire
Copy link
Contributor

mcmire commented May 5, 2023

The lint command takes around 10 seconds to run. This is fine when it's run on CI, but it feels a bit uncomfortable when run as a part of a push.

Ideally, what we want is something like this:

  • Full run (check or fix):
    • Run ESLint on all *.js and *.ts files, omitting those in .gitignore
    • Run Prettier on all *.json, *.md, and *.yml files, omitting those in .gitignore, as well as CHANGELOG.mds in any packages, CHANGELOG.old.md, and .yarnrc.yml
  • Partial run (changed files only):
    • Run ESLint on all changed *.js and *.ts files, omitting those in .gitignore
    • Run Prettier on all changed *.json, *.md, and *.yml files, omitting those in .gitignore, as well as CHANGELOG.mds in any packages, CHANGELOG.old.md, and .yarnrc.yml

More details below:


The Snaps monorepo attempts to reduce the runtime by using lint-staged to only linting files that have been staged. There are two wrinkles with the approach that the Snaps monorepo takes that would prevent us from copying it to this repo, however.

First, lint-staged is designed to be paired with a pre-commit hook, but we use a pre-push hook instead, so we'd need a different solution. So minimally, we could use something like lint-changed instead.

However, lint-changed (and lint-staged, for that matter) assume that you don't want to customize the set of files that are processed when running a full lint. For ESLint, that's not a problem, as we can get away with asking it to lint the whole project and then use .gitignore to filter out undesired files. Prettier, however, doesn't use .gitignore by default, so we tell it to use .gitignore, but then we also filter out more files on top of that.

So if we were to start using lint-changed, we'd have to have a separate way of running Prettier. We'd need a way to run file paths that lint-changed has reported as changed, but continue to filter out undesired files that aren't covered by .gitignore.

The Prettier team has made this somewhat easier by allowing multiple ignore file paths to be passed: prettier/prettier#14332. So we could pull out the list of files that we're excluding to a .prettierignore and then instruct the prettier command to use that in addition to .gitignore.

However, one problem would remain: we'd have to accommodate two ways of calling — and configuring — our lint commands.

Say we were able to reduce package.json to something like this:

{
  "scripts": {
    "lint": "yarn lint:js && yarn lint:misc --check && yarn constraints",
    "lint:fix": "yarn lint:js --fix && yarn lint:misc --write && yarn constraints --fix",
    "lint:js": "eslint . --cache --ext js,ts",
    "lint:misc": "prettier '**/*.json' '**/*.md' '**/*.yml' --ignore-path .gitignore --ignore-path .prettierignore"
  }
}

Now say we were to add lint-changed. That might look something like this:

{
  "scripts": {
    "lint": "yarn lint:js --check && yarn lint:misc --check && yarn constraints",
    "lint:fix": "yarn lint:js --fix && yarn lint:misc --write && yarn constraints --fix",
    "lint:js": "eslint --cache --ext js,ts .",
    "lint:misc": "prettier --ignore-path .gitignore --ignore-path .prettierignore '**/*.json' '**/*.md' '**/*.yml'"
  },
  "lint-staged": {
    "*.{js,ts}": "eslint --cache --ext js,ts",
    "*.{json,md,yml}": "prettier --ignore-path .gitignore --ignore-path .prettierignore"
  }
}

(We don't specify any file paths for lint-staged because it fills them in for us.)

Note how we have to specify two ways of running the eslint and prettier commands, and we have to specify acceptable file extensions twice. It'd be nice if we could just say:

{
  "scripts": {
    "lint": "scripts/lint.sh --check",
    "lint:fix": "scripts/lint.sh --fix"
  },
  "lint-staged": {
    "*": "scripts/lint.sh --check"
  }
}

To get this to work, such a lint script would have to:

  • figure out which file extensions go with which command, and specify them in the way that the tool understands
  • accept a set of file patterns, or else default to . if no file patterns were specified, and pass the result off to the tool

So this ticket depends on the release of Prettier 3.0, but it also depends on writing such a lint script.

@mcmire
Copy link
Contributor Author

mcmire commented May 8, 2023

I've started a branch to work on this: https://github.com/MetaMask/core/tree/use-lint-changed

@mcmire mcmire self-assigned this May 6, 2024
@mcmire mcmire linked a pull request May 6, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants