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

Added pre-commit hook #1024

Merged
merged 9 commits into from
Jun 13, 2023
Merged

Added pre-commit hook #1024

merged 9 commits into from
Jun 13, 2023

Conversation

Smit-Parmar
Copy link
Contributor

Issues

Resolve #925

Summary

Added back Pre-Commit Hook

Checklist

  • Tests have been included for all bug fixes or added functionality.
  • The changelog.rst has been updated with any significant changes.

@rmorshea
Copy link
Collaborator

rmorshea commented Jun 8, 2023

A couple areas for improvement:

  • It might make sense to have hatch actually run pre-commit rather than the other way around. Dong it this way, where pre-commit tells hatch to execute the set of linting tools might introduce a fair bit of latency to the typical git commit workflow that could be avoided. I could be wrong about that though.
  • We'd also like to run the steps that are presently in lint-js with pre-commit as well.
  • We want pre-commit to fix what it can. Presently that involves passing a --fix flag to hatch run lint-*

@Smit-Parmar
Copy link
Contributor Author

Smit-Parmar commented Jun 9, 2023

@rmorshea Opinion on respective point

  1. If pre-commit is running hatch command then it will be automated process whenever someone commits . Latency will be negligible I guess.
  2. Sure we can add lint-js
  3. We can add --fix as well

Planning to execute pre-commit in following order

  • Fix Python lint.
  • Check Python Lint
  • Fix JS lint.
  • Check JS lint.
    Let me know your thoughts on this.

@rmorshea
Copy link
Collaborator

rmorshea commented Jun 9, 2023

We should make it so you don't have to run "fix" and then "check". The "fix" command should return a non-zero exit code if it needs to make corrections. For example, Ruff has a flag for this (--exit-non-zero-on-fix). I believe black does this by default. Not entirely sure if the JS tools are like Ruff (need a flag) or Black (non-zero on fix by default).

@Smit-Parmar
Copy link
Contributor Author

@rmorshea Actually I tried using --exit-non-zero-on-fix even without using it pre-commit will fail even if code has been auto formatted by --fix.

    repo: https://github.com/astral-sh/ruff-pre-commit
    # Ruff version.
    rev: 'v0.0.272'
    hooks:
      - id: ruff
        args: [--fix, --exit-non-zero-on-fix]

pre-commit will keep failing unless 4 of the steps completed successfully. What's your suggestion should we make pre-commit like above example (without hatch) or use hatch?

@rmorshea
Copy link
Collaborator

rmorshea commented Jun 9, 2023

I'll take a closer look at this shortly.

@rmorshea
Copy link
Collaborator

rmorshea commented Jun 10, 2023

I think we can proceed using hatch run lint-<lang> --fix. Can you add pre-commit as a dependency in the root pyproject.toml file?

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@rmorshea rmorshea merged commit 1e31419 into reactive-python:main Jun 13, 2023
17 checks passed
@Archmonger
Copy link
Contributor

@rmorshea We still need to enable precommit on the repo

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.

Add Back Pre-Commit Hooks
4 participants