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 ruff as default linting + formatting option #387

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

chrisjkuch
Copy link
Contributor

@chrisjkuch chrisjkuch commented Jun 19, 2024

Closes #374
Closes #388

This PR adds ruff as the default linting and formatting option for new ccds projects.

Implementation notes

  • I've slightly altered the Makefile command tests so that they now run within each venv harness. I check to ensure that linting and formatting run without error and that nothing is reformatted.
    • For whatever reason, I found that ruff piped to stdout and flake8/black/isort piped to stderr. Maybe there's a nuance here or I've done something wrong?
  • I've cleaned up the generated pyproject.toml and removed setup.cfg if using ruff since it's no longer needed.
  • I didn't stray very far from the default ruff config, changing only the line length. If we are opinionated about the default set of linting / formatting options, happy to add those, but I have no strong preferences.

Discussion

As mentioned, this PR makes ruff the default option. I think there's good arguments for this that mostly boil down to "it's simpler + faster":

  • one tool for linting + formatting so fewer dependencies and simpler config
  • no setup.cfg file so everything can live in pyproject.toml
  • ruff is quite fast

However, ruff is still (relatively) new, whereas flake8/black/isort is more mature, and most projects won't notice the speed difference until they get quite large. I could be persuaded that we should introduce ruff without making it the default option.

@chrisjkuch chrisjkuch added this to the v2.1 milestone Jun 19, 2024
@chrisjkuch chrisjkuch requested review from pjbull and jayqi June 19, 2024 16:51
## Format source code with ruff
.PHONY: format
format:
ruff check --select I --fix {{ cookiecutter.module_name }}
Copy link
Member

Choose a reason for hiding this comment

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

I don't like --select I here. We should use the fixable or unfixable configuration in pyproject.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed on removing --select. Do you have an opinion on whether we should keep the default (everything is fixable) or specify fixable vs unfixable categories?

Copy link
Member

Choose a reason for hiding this comment

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

In my other projects so far, I've been setting F as unfixable and it's been working for me so far. I haven't looked that hard at the rulesets though.

https://github.com/drivendataorg/erdantic/blob/664a3f71e22225635bc95e26578312c9b19dd363/pyproject.toml#L54-L60

{{ cookiecutter.repo_name }}/Makefile Outdated Show resolved Hide resolved
@jayqi
Copy link
Member

jayqi commented Jun 20, 2024

I'm in favor of having the default to be Ruff. It's clear that Ruff is the emerging standard, and they describe themselves as production-ready.

There are major projects that are fully using Ruff for linting and formatting.

Lots of other major projects (e.g., pip, matplotlib, PyTorch) are using Ruff for linting over flake8, though are still using black for now.

@szapp
Copy link

szapp commented Oct 11, 2024

It seems that slight adjustments to the linter rules are necessary to ensure similar behavior as the flake8/black/isort alternative. Most of the rules that flake8 ignores in the setup.cfg are ignored in the defaults of ruff as well, except for E731 if I am not mistaken.

If I am not wrong, these additional lines in pyproject.toml should do.

[tool.ruff.lint]
ignore = [
  "E731"  # lambda-assignment: Do not assign a lambda expression, use a def
]

Update: Additionally, the I (isort) rules should be added according to the readme of the charliermarsh.ruff vscode extension in order to enable organization of imports. It's not enabled by default.

[tool.ruff.lint]
ignore = ["E731"]  # Ignore lambda-assignment
extend-select = ["I"]  # Add sorting imports (isort ruleset) to defaults

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ruff lint configuration names are incorrect
3 participants