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

Saner linters #58

Closed
wants to merge 1 commit into from
Closed

Saner linters #58

wants to merge 1 commit into from

Conversation

crusaderky
Copy link
Contributor

Thorough review of the linters in use. This was triggered by #53, where I found many unresolvable conflicts among the linters.

@@ -35,11 +35,8 @@ jobs:
pixi-version: v0.39.0
cache: true
environments: lint
- name: Run Pylint, Mypy & Pyright
run: |
pixi run -e lint pylint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redundant and incompatible with ruff

Choose a reason for hiding this comment

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

Pylint is also included in ruff, so why would it be incompatible?

Choose a reason for hiding this comment

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

there are many pylint rules that ruff hasn't implemented: astral-sh/ruff#970

Copy link
Contributor Author

@crusaderky crusaderky Dec 10, 2024

Choose a reason for hiding this comment

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

A simple example was

class at:

pylint complains that classes need to be capitalized;
but if you add a # noqa to silence pylint, then ruff fails with an "unnecessary noqa" error

Copy link
Collaborator

Choose a reason for hiding this comment

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

the way to silence pylint is # pylint: disable=invalid-name

run: |
pixi run -e lint pylint
pixi run -e lint mypy
pixi run -e lint pyright
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't have pyright AND mypy in the same project. They will eventually result conflicting with each other, and in fact they did so heavily in #53.
Chose mypy as I found pyright extremely obnoxious and factually wrong in too many cases.

Copy link

@jorenham jorenham Dec 10, 2024

Choose a reason for hiding this comment

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

Can't have pyright AND mypy in the same project. They will eventually result conflicting with each other, and in fact they did so heavily in #53.

This isn't true at all, and all of my typed projects are proof of this.
In my experience, there are indeed situations where mypy and pyright behave differently. In at least 99% of these cases, this is caused by one of the >1200 confirmed bugs in mypy that aren't present in pyright: https://github.com/erictraut/mypy_issues

Chose mypy as I found pyright extremely obnoxious and factually wrong in too many cases.

Do you have an example of this?

@@ -70,9 +70,7 @@ array-api-extra = { path = ".", editable = true }

[tool.pixi.feature.lint.dependencies]
pre-commit = "*"
pylint = "*"
basedmypy = "*"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced basedmypy with mypy. I think it is an extremely bad idea to diverge from PEPs. Namely, IDE plugins will not support this.

Choose a reason for hiding this comment

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

This is untrue. All mypy IDE plugins and other CI tools also work with basedmypy.

In fact, mypy diverges more from the typing specification that basedmypy does. It also has significantly more bugs.

@crusaderky crusaderky mentioned this pull request Dec 10, 2024
@lucascolley
Copy link
Collaborator

Overall I'm -1 on this change. I'm happy to investigate conflicts.

Can't have pyright AND mypy in the same project.

That isn't true, it is done successfully in @jorenham's scipy-stubs.

@rgommers
Copy link
Member

Quick comment: conceptually this PR seems right. Best to pick 1 linter and 1 type checker, everything else is pretty much unworkable once the code starts getting more complex. I'd personally pick ruff (for autofix and well-maintained) and pyright (much better maintained and less buggy than mypy), but it doesn't really matter what the choices are as long as there's only one tool per type of check.

@lucascolley
Copy link
Collaborator

everything else is pretty much unworkable once the code starts getting more complex.

Let me take a look at resolving the issues @crusaderky is facing. If it really is unworkable, then of course, we should make changes to make it workable.

@jorenham
Copy link

Best to pick 1 linter and type checker

In case of linters I'd agree with this, unless they're actually linting different things. In my project I always use (only) ruff for python, but I also always markdownlint, and in most cases also repo-review. These are all disjoint, so I see no problem with that.

I don't agree that you should only should pick one type checker. I always use both mypy and pyright in my projects, because I want the users to also be able to use both, which requires me to verify that both type-checkers actually understand the typing annotations.
Mypy and pyright certainly overlap, but also differ in many situations. This is mostly because mypy has >1200 bugs that pyright doesn't have (source). But unfortunately, more than half of the users use mypy at the moment, so I can't just ignore it.

@lucascolley
Copy link
Collaborator

RE: pylint, the Scientific Python Development guide says

PyLint is very opinionated, with a high signal-to-noise ratio. However, by limiting the default checks or by starting off a new project using them, you can get some very nice linting, including catching some problematic code that otherwise is hard to catch.

We currently use the recommended limits on the default checks. If there are conflicts with ruff then it shouldn't be difficult to extend that list.

@KotlinIsland
Copy link

typeshed uses both mypy and pyright

Copy link
Collaborator

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

it looks like this is workable without changing the existing config - see gh-59

@crusaderky crusaderky deleted the lint branch December 17, 2024 14:18
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.

5 participants