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 pre-commit and use ruff #1659

Merged
merged 3 commits into from
Aug 11, 2024
Merged

Add pre-commit and use ruff #1659

merged 3 commits into from
Aug 11, 2024

Conversation

JasonGrace2282
Copy link
Member

@JasonGrace2282 JasonGrace2282 commented Apr 7, 2024

A similar change has been merged into Tin, see tjcsl/tin#21

If you are reviewing this PR, I suggest looking at it commit-by-commit instead of looking at the diff all at once.

Proposed changes

  • Use pre-commit for linting/formatting
  • Ideally maintainers will add pre-commit ci so that if the pre-commit fails, fixes will be pushed without having to run scripts.
    • Otherwise when creating a virtual environment, you can run pre-commit install and it should run pre-commit locally whenever you commit
  • Change from black+autopep8 to ruff

Brief description of rationale

Running scripts is annoying lol

Summary of Changes

  • Add .pre-commit-config.yaml
    • Also adds some config for pre-commit ci if it is decided to be added
  • Add pre-commit to dependencies
  • Changes test pipeline to run pre-commit run --all-files instead of scripts for formatting/linting
  • Deprecate the format scripts

Note

PR #1673 should be merged before this to see if all checks pass.
I just tested locally, there are about ~11 files that are changed after merging it. Once it's merged I'll push a commit updating this PR with those 11 files.

Progress

@JasonGrace2282 JasonGrace2282 changed the title Add .pre-commit Add pre-commit Apr 7, 2024
@JasonGrace2282 JasonGrace2282 force-pushed the pre-commit branch 5 times, most recently from a7e9096 to eaf426f Compare April 7, 2024 18:22
@JasonGrace2282
Copy link
Member Author

JasonGrace2282 commented Apr 7, 2024

To reiterate what I said on matterless:
The current problem is (to the best of my knowledge) that black and autopep8 interfere with each other. Black will format some files, then autopep8 will go in and fix those. This was fine in scripts where it simply merged both outputs (with autopep8 taking priority), but with pre-commit it counts a check as a fail if it modifies files. As such, both black and autopep8 will fail each run (black being unhappy with autopep8 and vice versa).
The possible solutions to this I can think of include:

  • Remove autopep8 and stick with black
  • Remove black and only use autopep8
  • Don't use either, and use one formatter (something like ruff)
    • This is my preferred option due to ruff's speed, similarity to black, and a built in linter (removing the need for flake8).
  • Do something like:
- repo: local
  hooks:
    id: format
    entry: ./scripts/format.sh && test -z "$(git status --porcelain=v1)"

(which I'm not sure would work with pre-commit ci)

@alanzhu0 @NotFish232 would be nice to get a second opinion on this, when you find the time of course ;)

@JasonGrace2282 JasonGrace2282 force-pushed the pre-commit branch 7 times, most recently from 3101b09 to 9246b40 Compare April 8, 2024 20:45
@JasonGrace2282 JasonGrace2282 force-pushed the pre-commit branch 3 times, most recently from 8cadb96 to df7411e Compare May 2, 2024 17:08
@JasonGrace2282 JasonGrace2282 mentioned this pull request May 3, 2024
@JasonGrace2282 JasonGrace2282 force-pushed the pre-commit branch 7 times, most recently from e415d0c to 3d30b97 Compare May 4, 2024 14:04
@JasonGrace2282
Copy link
Member Author

JasonGrace2282 commented May 4, 2024

I think this PR is mostly ready to be reviewed.
Some benchmarks on my system:

  • time pre-commit run --all-files takes 11.32 seconds (user)
  • time ruff check intranet/**/*.py takes 0.45 seconds (user)
  • time ruff lint intranet/**/*.py takes 0.04 seconds (user)

Since the diff is very large, I moved it into a separate PR: #1673

@JasonGrace2282 JasonGrace2282 force-pushed the pre-commit branch 2 times, most recently from c487cdd to 9807bed Compare May 4, 2024 14:22
@JasonGrace2282
Copy link
Member Author

JasonGrace2282 commented May 4, 2024

In order to get the tests to pass (specifically flake8) in #1673, I had to make some changes that don't agree with the ruff (or black for that matter!). As such, the changes in that PR aren't 100% reflective of the changes needed here (but it's a pretty small and easy to review diff after that PR is merged).

On another note, if you're wondering why there is a formatting CI run that never completes, it's because it's marked as required in the repository settings.

@JasonGrace2282 JasonGrace2282 changed the title Add pre-commit Add pre-commit and use ruff May 4, 2024
@JasonGrace2282 JasonGrace2282 force-pushed the pre-commit branch 2 times, most recently from ee32239 to 7a6dc1e Compare May 4, 2024 18:56
@JasonGrace2282 JasonGrace2282 force-pushed the pre-commit branch 6 times, most recently from 2f1bc97 to 7f643b8 Compare June 18, 2024 02:06
@coveralls
Copy link

Coverage Status

coverage: 79.412%. remained the same
when pulling c5724a2 on JasonGrace2282:pre-commit
into 1d1ff53 on tjcsl:dev.

@coveralls
Copy link

Coverage Status

coverage: 79.503% (+0.04%) from 79.468%
when pulling 2e5e6c1 on JasonGrace2282:pre-commit
into f164994 on tjcsl:dev.

@coveralls
Copy link

Coverage Status

coverage: 79.447% (-0.02%) from 79.468%
when pulling 2e5e6c1 on JasonGrace2282:pre-commit
into f164994 on tjcsl:dev.

@coveralls
Copy link

Coverage Status

coverage: 79.447% (-0.02%) from 79.468%
when pulling 2e5e6c1 on JasonGrace2282:pre-commit
into f164994 on tjcsl:dev.

This removes the need for black, autopep8, and isort

Also includes the configuration for pre-commit ci, if
it is decided that it should be added.
@coveralls
Copy link

Coverage Status

coverage: 79.371%. remained the same
when pulling bab04ac on JasonGrace2282:pre-commit
into 47727af on tjcsl:dev.

@alanzhu0 alanzhu0 merged commit fb2d797 into tjcsl:dev Aug 11, 2024
5 checks passed
@JasonGrace2282 JasonGrace2282 deleted the pre-commit branch August 11, 2024 21:51
JasonGrace2282 added a commit to JasonGrace2282/ion that referenced this pull request Aug 25, 2024
This commit autodocuments Ion apps (removing the need for a script), and
fixes many of the formatting errors in the current docs.
It also updates the docs for the formatting/linting CI, which was
updated in tjcsl#1659
JasonGrace2282 added a commit to JasonGrace2282/ion that referenced this pull request Aug 25, 2024
This commit autodocuments Ion apps (removing the need for a script), and
fixes many of the formatting errors in the current docs.
It also updates the docs for the formatting/linting CI, which was
updated in tjcsl#1659
JasonGrace2282 added a commit to JasonGrace2282/ion that referenced this pull request Aug 25, 2024
This commit autodocuments Ion apps (removing the need for a script), and
fixes many of the formatting errors in the current docs.
It also updates the docs for the formatting/linting CI, which was
updated in tjcsl#1659
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.

4 participants