Skip to content

Add pre commit config#19

Merged
dosisod merged 2 commits intodosisod:masterfrom
EFord36:add-pre-commit-config
Sep 29, 2022
Merged

Add pre commit config#19
dosisod merged 2 commits intodosisod:masterfrom
EFord36:add-pre-commit-config

Conversation

@EFord36
Copy link
Contributor

@EFord36 EFord36 commented Sep 29, 2022

Addresses #10

Sets up a pre-commit hook in the repo so that users can use the repo to set up refurb as a pre-commit hook in their projects.

my .pre-commit-config.yaml file contents while testing (I also used the pre-commit try-repo command while I was working this out:

repos:
  - repo: https://github.com/EFord36/refurb
    rev: c4d1282
    hooks:
      - id: refurb

If this was merged into the base repo, it would just be:

repos:
  - repo: https://github.com/dosisod/refurb
    rev: SOME_REVISION_HERE
    hooks:
      - id: refurb

I've tested this on single files with one issue, multiple issues, multiple files (one with multiple issues), multiple clean files, pre-commit run -a refurb to test it works running over a codebase of existing files (admittedly a small one) - in the cases of all 'clean', some with problems. All work fine for me on a test repo.

One tricky caveat I noticed - require_serial: true seems to be a good idea, otherwise the built-in pre-commit multiprocessing means the 'Run refurb --explain ERR to further explain an error. Use --quiet to silence this message' get output multiple times in unpredictable ways when handling multiple files:

test.py:2:1 [FURB113]: Use `x.extend(...)` instead of repeatedly calling `x.append()`
test.py:10:9 [FURB126]: Replace `else: return x` with `return x`
test_copy_1.py:2:1 [FURB113]: Use `x.extend(...)` instead of repeatedly calling `x.append()`

Run `refurb --explain ERR` to further explain an error. Use `--quiet` to silence this message
test_copy_2.py:2:1 [FURB113]: Use `x.extend(...)` instead of repeatedly calling `x.append()`

Run `refurb --explain ERR` to further explain an error. Use `--quiet` to silence this message

Another way to address this would be to remove the require_serial: true option and add an args option to the file so that --quiet is passed - I didn't want to do this by default without discussing with you though. Happy to change if that's what you prefer (it might be more performant - a quick scan of refurb's main.py looks like it just loops over files rather than having multiprocessing?).

If you did accept this PR, I could also ask if it's possible to add refurb into the set of supported hooks in pre-commit.com by forking the repo for that site and adding your repo in if you were interested?

Final point - thanks for refurb - it looks really interesting and as someone who loves doing code reviews as well I appreciate the effort :)

Allows refurb to be installed as a pre-commit hook for users.
This seems to cause the 'explain' message at the bottom to be output
inconsistently.
@EFord36 EFord36 mentioned this pull request Sep 29, 2022
Copy link
Owner

@dosisod dosisod left a comment

Choose a reason for hiding this comment

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

Thank you for the work and testing you put into making this PR! I think this is a good addition, since it seems a lot more people will use Refurb if this is added.

In regards to the requires_serial issue, it will be best to keep it set to true. In my testing, it takes (slightly) longer to run in parallel, while only using up more CPU time. See this issue which shows a nice example. Since Mypy is the underlying type checker, we are at it's mercy when it comes to performance and caching. Running in serial also has the added benefit of making the "Use --explain ..." errors from appearing out of order.

Again, thank you for the PR, and thank you for using Refurb!

@dosisod
Copy link
Owner

dosisod commented Sep 29, 2022

And yes, submitting a PR to the pre-commit hook page is fine by me

@dosisod dosisod merged commit 4a6cb6b into dosisod:master Sep 29, 2022
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.

2 participants