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

Clarify ruff functionality. #403

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Clarify ruff functionality. #403

merged 3 commits into from
Feb 9, 2024

Conversation

delucchi-cmu
Copy link
Contributor

Change Description

Closes #395
Closes #384

Checklist

  • This PR is meant for the lincc-frameworks/python-project-template repo and not a downstream one instead.
  • This change is linked to an open issue
  • This change includes integration testing, or is small enough to be covered by existing tests

@delucchi-cmu delucchi-cmu requested a review from hombit February 7, 2024 20:33
Comment on lines 97 to 116
{%- if 'ruff_lint' in enforce_style %}
- repo: https://github.com/astral-sh/ruff-pre-commit
# Ruff version.
rev: v0.1.3
hooks:
- id: ruff
name: Format and lint code using ruff
name: Lint code using ruff
types_or: [ python, pyi, jupyter ]
args: ["check"]
{%- endif %}
{%- if 'ruff_format' in enforce_style %}
- repo: https://github.com/astral-sh/ruff-pre-commit
# Ruff version.
rev: v0.1.3
hooks:
- id: ruff
name: Format code using ruff
types_or: [ python, pyi, jupyter ]
args: ["format", "--check"]
{%- endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why the hook's readme proposes to put hooks in this order. If we do this, the linter would fail for some bad formatting (for example long lines), before formatter runs. I suggest to swap the order, so formatter runs first and fixes some linting issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I agree with the ruff folks on this one!

Since the pre-commit is mostly about warning, and not about fixing the changes, I'm a fan of the lint-first-then-format ordering. This would also alert sooner about "code smell" type problems, before alerting about more minor formatting issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is that we can automatically fix formatting with the first hook, instead of fail

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll move this discussion to a new issue. Thank you!

@hombit hombit self-requested a review February 9, 2024 14:45
@delucchi-cmu delucchi-cmu merged commit c611fbc into main Feb 9, 2024
13 checks passed
@delucchi-cmu delucchi-cmu deleted the issue/395/ruff branch February 9, 2024 14:49
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 an option to add ruff as a default formatter pre-commit ruff hook name is misleading
2 participants