Skip to content

DEV: align bin/lint to use lefthook #34247

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 13, 2025
Merged

DEV: align bin/lint to use lefthook #34247

merged 3 commits into from
Aug 13, 2025

Conversation

SamSaffron
Copy link
Member

@SamSaffron SamSaffron commented Aug 12, 2025 •
edited
Loading

  • Align lefthook linting with linting CI does
  • Call lefthook from bin/lint to handle linting (centralizes implementation)

@SamSaffron SamSaffron changed the title fix lint DEV: align bin/lint to use lefthook Aug 12, 2025
CvX
CvX reviewed Aug 12, 2025
View reviewed changes
@SamSaffron
Copy link
Member Author

spec failure unrelated

@pento
Copy link
Member

pento commented Aug 13, 2025 •
edited
Loading

At this point, it seems like bin/lint has largely been reduced to acting as a wrapper around lefthook run, feeding it different files based on a handful of scenarios. It looks like this could be done pretty cleanly just within lefthook.yml. For example, :

lints:
  parallel: true
  files: git ls-files --cached
  commands: &lint_commands
    rubocop:
      glob: "*.rb"
      run: bundle exec rubocop
    prettier:
      run: pnpm lint:prettier
    eslint:
      run: pnpm lint:js
    ember-template-lint:
      run: pnpm lint:hbs
    stylelint:
      run: pnpm lint:css
    yaml-syntax:
      glob: "*.{yaml,yml}"
      # database.yml is an erb file not a yaml file
      exclude: "database.yml"
      run: bundle exec yaml-lint {files}
    i18n-lint:
      glob: "**/{client,server}.en.yml"
      run: bundle exec ruby script/i18n_lint.rb {files}
    glint:
      run: pnpm glint -p jsconfig.json --noEmit

lint-staged:
  parallel: true
  files: git diff --name-only --cached
  commands: *lint_commands

lint-recent:
  parallel: true
  files: "git log -50 --name-only --pretty=format:"
  commands: *lint_commands

lefthook run lints would run across all files, lefthook run lint-staged would run across staged files, and lefthook run lint-recent would run across the last 50 commits, etc.

@SamSaffron
Copy link
Member Author

I hear you @pento, no objection to pulling more out of bin/lint into lefthook, but fundamentally we are never going to get a clear help like:

bin/lint --help
Usage: bin/lint [options] [files...]
    -h, --help                       Show this help message
    -f, --fix                        Attempt to automatically fix issues
    -r, --recent                     Lint recently changed files (last 50 commits)
        --staged                     Lint only staged files
        --unstaged                   Lint only unstaged files
        --wip                        Lint work-in-progress: staged + unstaged + files changed since main
    -v, --verbose                    Show verbose output

Examples:
  bin/lint                        # Lint all files
  bin/lint --recent               # Lint recently changed files
  bin/lint --staged               # Lint only staged files
  bin/lint --unstaged             # Lint only unstaged files
  bin/lint --wip                  # Lint staged + unstaged + files changed since main
  bin/lint --fix app.rb file2.js  # Fix specific file/s
  bin/lint app/models/*.rb        # Lint multiple files

lefthook has an awkward CLI with awkward help.

pnpm lefthook run pre-commit --file test.rb --file test2.rb

I am fine for bin/lint to be an alias, this a feature here.

@pento
Copy link
Member

pento commented Aug 13, 2025

That's a fair point, the ability to show a custom help message is a big benefit.

@SamSaffron SamSaffron merged commit 720cf1c into main Aug 13, 2025
15 of 16 checks passed
@SamSaffron SamSaffron deleted the fix-lint branch August 13, 2025 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CvX CvX CvX left review comments

@pento pento pento approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SamSaffron @pento @CvX