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

chore(python): Add a lint-only Makefile option #14602

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

alexander-beedie
Copy link
Collaborator

@alexander-beedie alexander-beedie commented Feb 20, 2024

More cleanly separates the linting and formatting options in theMakefile (it was never clear to me why "fmt" would also trigger linting as they are not the same thing)

Update; PR now just adds make lint (no change to fmt).

Example

make lint  # new; runs only the ruff/mypy linting ✔️

@github-actions github-actions bot added the enhancement New feature or an improvement of an existing feature label Feb 20, 2024
@alexander-beedie alexander-beedie added the build Changes that affect the build system or external dependencies label Feb 20, 2024
Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

I don't think there's a clear distinction here, because the ruff linter also does formatting, e.g. it will autofix bad lints which will end up reformatting the code.

I don't like running pre-commit because clippy is slow. make fmt runs all the Python-side linting/formatting, which is great and all I need usually.

You can run mypy separately if you need to (I do that regularly if only mypy fails).

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Feb 20, 2024

I don't think there's a clear distinction here, because the ruff linter also does formatting, e.g. it will autofix bad lints which will end up reformatting the code.

The "make fmt" and "make lint" options observe the same difference as ruff format and ruff check, which seems fairly obvious to me. Fixing lints isn't really formatting, even if it can result in a few such cases as a side-effect of the chosen rules.

I don't like running pre-commit because clippy is slow. make fmt runs all the Python-side linting/formatting, which is great and all I need usually.

And if you haven't done a cargo clean for a while it can go a little ballistic; I just scrubbed ~96GB and it settled down again 🤣

@stinodego
Copy link
Member

It's not clear to me when you would need to run the two commands separately?

All I know is that I don't want my current make fmt to change. Feel free to add a make lint though if you really need it.

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Feb 20, 2024

It's not clear to me when you would need to run the two commands separately?

When I want to check lint before going for pre-commit. I don't need a separate formatting step as my IDE keeps that in line automatically on code save (there's a ruff plugin for PyCharm that can read your project's settings).

Feel free to add a make lint though if you really need it.

Will do that and revert for fmt👌
(Though still feels odd for a formatting directive to trigger linting ;)

@alexander-beedie alexander-beedie changed the title feat(build): split out "fmt" and "lint" options in the Makefile feat(build): add a lint-only Makefile option Feb 20, 2024
@alexander-beedie
Copy link
Collaborator Author

Feel free to add a make lint though if you really need it.

Done 👍

@stinodego stinodego changed the title feat(build): add a lint-only Makefile option chore(python): Add a lint-only Makefile option Feb 20, 2024
@stinodego stinodego removed build Changes that affect the build system or external dependencies enhancement New feature or an improvement of an existing feature labels Feb 20, 2024
@github-actions github-actions bot added internal An internal refactor or improvement python Related to Python Polars labels Feb 20, 2024
@stinodego stinodego merged commit 816f4b5 into pola-rs:main Feb 20, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants