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

Replace flake8, isort and pydocstyle with ruff #541

Merged
merged 15 commits into from
Jun 25, 2024
Merged

Replace flake8, isort and pydocstyle with ruff #541

merged 15 commits into from
Jun 25, 2024

Conversation

gruberb
Copy link
Member

@gruberb gruberb commented Jun 24, 2024

References

JIRA: DISCO-2165

Description

Replacing flake8, isort and pydocstyle with ruff

  • flake8 is a linter, which can be replaced with ruff check
  • isort can sort imports, which is not possible with a single command yet. We have to use ruf check --fix && ruff format (there is an open discussion to have a combined command: Unified command for linting and formatting astral-sh/ruff#8232)
  • pydocstyle is supported out of the box, and the configuration can be taken over to ruff.

I also attempted to upgrade dependencies which are still using flake8, isort or pydocstyle in our version, and have since updated to ruff.

  • Packages still using flake8
    • commonmark (imported through rich and scalene. commonmark is also not maintained anymore, and markdown-it-py recommended.)
    • typer upgraded to version 0.11.0 which replaces flake8 with ruff
    • uvloop (through uvicorn)
    • bandit
  • Packages still using isort
    • None
  • Packages still using pydocstyle
    • None

Changes in linting

This commit holds the changes from ruff check: 527dd5b.

Challenges

ruff is not a 1:1 replacement for isort, which means our "just do sorting with make isort doesn't truly work anymore. I kept the step, but the story is slightly more complicated: https://docs.astral.sh/ruff/formatter/#sorting-imports

Further reading

To discuss

We should be able to replace black with ruff as well: https://docs.astral.sh/ruff/faq/#is-the-ruff-linter-compatible-with-black

PR Review Checklist

Put an x in the boxes that apply

  • This PR conforms to the Contribution Guidelines
  • The PR title starts with the JIRA issue reference, format [DISCO-####], and has the same title (if applicable)
  • [load test: (abort|warn)] keywords are applied (if applicable)
  • Documentation has been updated (if applicable)
  • Functional and performance test coverage has been expanded and maintained (if applicable)

@gruberb gruberb changed the title Ruff Replace flake8, isort and pydocstyle with ruff Jun 24, 2024
@gruberb gruberb marked this pull request as ready for review June 24, 2024 16:24
Copy link
Contributor

@ncloudioj ncloudioj left a comment

Choose a reason for hiding this comment

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

Thanks for taking a crack at it! Looks good, I just have a few comments about re-organizing those Make targets and possibly replace black with ruff format. Let me know what you think.

.flake8 Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@ncloudioj ncloudioj left a comment

Choose a reason for hiding this comment

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

Looks like ruff-doc doesn't pick up the ignored rules.

Makefile Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@gruberb
Copy link
Member Author

gruberb commented Jun 24, 2024

Looks like ruff-doc doesn't pick up the ignored rules.

Yeah, I am trying a few things locally to see why. Based on the docs, it should.

@ncloudioj
Copy link
Contributor

Yeah, I am trying a few things locally to see why. Based on the docs, it should.

Looks like if I dropped the --select D from the ruff check for "ruff-doc" and added tool.ruff.lint.select = ["D"], it worked. Maybe if you do --select D, you will also need to specify --ignore XYZ from the CLI?

@ncloudioj
Copy link
Contributor

This is what I use, I think we will need to add more items to select.

[tool.ruff.lint]
extend-select = ["D212"]
select = ["D"]
ignore = [
  # D105 Docstrings for magic methods
  "D105",
  # D107 Docstrings for __init__
  "D107",
  # D203 as it conflicts with D211 https://github.com/PyCQA/pydocstyle/issues/141
  "D203",
  # D205 1 blank line required between summary line and description, awkward spacing
  "D205",
  # D400 First line should end with a period, doesn't work when sentence spans 2 lines
  "D400",
]
extend-ignore = ["E203"]
per-file-ignores = {"__init__.py" = ["F401"]}
pydocstyle.convention = "pep257"

@gruberb
Copy link
Member Author

gruberb commented Jun 24, 2024

Looks like if I dropped the --select D from the ruff check for "ruff-doc" and added tool.ruff.lint.select = ["D"], it worked. Maybe if you do --select D, you will also need to specify --ignore XYZ from the CLI?

This is the part I am not sure. We either change how pyproject.toml is configured (and add a few to lint.select), or we add it explicitly in the Makefile (what I have done with the latest push: $(POETRY) run ruff check --select D --ignore D105,D107,D203,D205,D400 $(APP_AND_TEST_DIRS)

I think I prefer to have everything in pyproject.toml, so we don't have to duplicate and at some point run into sync issues.

The downside is that we will always check docstrings, and just basically have one big check, instead of specific checks for D, E etc.

@ncloudioj
Copy link
Contributor

Agreed, let's not duplicate those configs in the Makefile.

The downside is that we will always check docstrings, and just basically have one big check, instead of specific checks for D, E etc.

True, not ideal but ruff is fast ;). We can also merge ruff-doc into ruff-lint.

@gruberb gruberb requested a review from ncloudioj June 25, 2024 11:22
Copy link
Contributor

@ncloudioj ncloudioj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

pyproject.toml Outdated Show resolved Hide resolved
@ncloudioj
Copy link
Contributor

@gruberb FYI, you can also assign context-services as the group reviewer rather than assigning to individuals (unless you do want someone specifically to review), someone from that group would pick up the review :).

@gruberb gruberb added this pull request to the merge queue Jun 25, 2024
Merged via the queue into main with commit b485be4 Jun 25, 2024
8 checks passed
@gruberb gruberb deleted the ruff branch June 25, 2024 14:11
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