add: ruff linter by mavwolverine · Pull Request #293 · getappmap/appmap-python

mavwolverine · 2024-03-16T19:47:52Z

poetry run ruff check --fix appmap

https://docs.astral.sh/ruff/

apotterri · 2024-03-17T09:18:12Z

We're already using pylint for linting: https://github.com/getappmap/appmap-python/blob/master/tox.ini#L24 . I'm not opposed to switching to ruff, but if we do, we should remove pylint. This should probably also include a config file for ruff that more or less matches the pylint config.

apotterri · 2024-03-17T09:20:39Z

We're using semantic-release for appmap-python, so the subject lines for commits need to follow this format: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#-commit-message-format .

(Our own CONTRIBUTING.md really needs an update that mentions this....)

apotterri

Thanks very much for this.

apotterri · 2024-03-17T09:04:55Z

appmap/unittest.py


 if not Env.current.is_appmap_repo and Env.current.enables("unittest"):
    logger.debug("Test recording is enabled (unittest)")
-    import _appmap.unittest  # pyright: ignore pylint: disable=unused-import


This import needs to be retained, so it should be marked with # noqa: F401 rather than deleted.

apotterri · 2024-03-17T09:06:26Z

pyproject.toml

 #
 # So, if you'd like to run the tests outside of tox, run `pip install -r requirements-dev.txt` to
 # install it and the rest of the dev dependencies.
+ruff = "^0.3.3"


This should be in the dev dependencies (below).

mavwolverine · 2024-03-17T18:04:33Z

We're using semantic-release for appmap-python, so the subject lines for commits need to follow this format: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#-commit-message-format .

(Our own CONTRIBUTING.md really needs an update that mentions this....)

Updated the commit message. Hopefully chose the correct one.

mavwolverine · 2024-03-17T18:08:45Z

We're already using pylint for linting: https://github.com/getappmap/appmap-python/blob/master/tox.ini#L24 . I'm not opposed to switching to ruff, but if we do, we should remove pylint. This should probably also include a config file for ruff that more or less matches the pylint config.

Most of projects are now moving from pylint to ruff.
https://docs.astral.sh/ruff/faq/#how-does-ruffs-linter-compare-to-pylint

Also, the fastapi unused wsgimiddleware import, ruff highlighted it, but I didn't notice it as such in the pylint output.

This was a suggestion for replacing pylint with ruff and not both together.

I am not aware of other places where you might have to replace pylint in the dev environments/ CI etc.

apotterri · 2024-03-19T12:55:00Z

We're already using pylint for linting: https://github.com/getappmap/appmap-python/blob/master/tox.ini#L24 . I'm not opposed to switching to ruff, but if we do, we should remove pylint. This should probably also include a config file for ruff that more or less matches the pylint config.

Most of projects are now moving from pylint to ruff. https://docs.astral.sh/ruff/faq/#how-does-ruffs-linter-compare-to-pylint
Yup, seems like a good idea to me.

Also, the fastapi unused wsgimiddleware import, ruff highlighted it, but I didn't notice it as such in the pylint output.

This was a suggestion for replacing pylint with ruff and not both together.
Understood. I wasn't suggesting this either.

I am not aware of other places where you might have to replace pylint in the dev environments/ CI etc.
The main thing we're using pylint to try to keep the code from getting worse: https://github.com/getappmap/appmap-python/blob/master/tox.ini#L24 .

We need to retain this functionality if we switch to ruff, so this PR would need to include changes to do that, as well.

mavwolverine · 2024-03-19T14:42:49Z

@apotterri Just to confirm, this is executed when running tox? Also, what is the expected outcome here? Tests should fail if there is any pylint error? As pylint-exit only returns 1 on Fatal message issued or Usage error

apotterri · 2024-03-19T14:54:41Z

Yes, it's run when the tox environment is py310-web: https://app.travis-ci.com/github/getappmap/appmap-python/jobs/619305807#L579 .

pylint computes a score for the code, and has a setting that controls the minimum score: https://github.com/getappmap/appmap-python/blob/master/pylintrc#L3 . If the code that's getting built has a score that's below that threshold, pylint-exit returns non-zero.

mavwolverine · 2024-03-19T16:01:45Z

Yes, it's run when the tox environment is py310-web: https://app.travis-ci.com/github/getappmap/appmap-python/jobs/619305807#L579 .

pylint computes a score for the code, and has a setting that controls the minimum score: https://github.com/getappmap/appmap-python/blob/master/pylintrc#L3 . If the code that's getting built has a score that's below that threshold, pylint-exit returns non-zero.

Doesn't seem to be working that way.

# Specify a score threshold under which the program will exit with error.
fail-under=9.83

------------------------------------------------------------------
Your code has been rated at 9.80/10 (previous run: 9.80/10, +0.00)

The following messages were raised:

  - error message issued
  - warning message issued
  - refactor message issued
  - convention message issued

No fatal messages detected.  Exiting gracefully...

apotterri · 2024-03-19T16:58:54Z

That would be bug, then. It's intended to work that way, and I thought that was the description from pylint-exit.

apotterri · 2024-03-20T10:01:41Z

That would be bug, then. It's intended to work that way, and I thought that was the description from pylint-exit.

Looking again at the doc for pylint-exit, I see that I misread it. It doesn't clean to exit non-zero if the fail_under check fails. So, there's definitely a bug in the build that needs to be fixed.

apotterri · 2024-03-20T10:02:19Z

As far as replacing pylint with ruff....

I'm concerned about the statement here that ruff isn't a drop-in replacement for pylint: https://docs.astral.sh/ruff/faq/#how-does-ruffs-linter-compare-to-pylint , and especially that it does less type checking.

Also astral-sh/ruff#970 .

mavwolverine · 2024-03-25T17:05:11Z

Yes, ruff is used along with mypy. But at this time, adding type annotation for every variable might be a big task. We can leave adding ruff in backlog as a future task, but address the highlighted issues in a separate PR.

lightningRalf · 2025-05-04T08:43:04Z

So just to understand how to move this along is by adding mypy as a separate PR that does the type checking? And if this is implemented in a separate PR than this could move forward?

apotterri self-requested a review March 17, 2024 08:59

apotterri suggested changes Mar 17, 2024

View reviewed changes

build: ruff linter

42ae89d

mavwolverine force-pushed the ruff branch from 8352024 to 42ae89d Compare March 17, 2024 18:03

dividedmind force-pushed the master branch from 83f5126 to 4eaa6d3 Compare February 3, 2026 16:29

Conversation

mavwolverine commented Mar 16, 2024

Uh oh!

apotterri commented Mar 17, 2024

Uh oh!

apotterri commented Mar 17, 2024

Uh oh!

apotterri left a comment

Choose a reason for hiding this comment

Uh oh!

apotterri Mar 17, 2024

Choose a reason for hiding this comment

Uh oh!

mavwolverine Mar 17, 2024

Choose a reason for hiding this comment

Uh oh!

apotterri Mar 17, 2024

Choose a reason for hiding this comment

Uh oh!

mavwolverine commented Mar 17, 2024

Uh oh!

mavwolverine commented Mar 17, 2024

Uh oh!

apotterri commented Mar 19, 2024

Uh oh!

mavwolverine commented Mar 19, 2024

Uh oh!

apotterri commented Mar 19, 2024

Uh oh!

mavwolverine commented Mar 19, 2024

Uh oh!

apotterri commented Mar 19, 2024

Uh oh!

apotterri commented Mar 20, 2024

Uh oh!

apotterri commented Mar 20, 2024

Uh oh!

mavwolverine commented Mar 25, 2024

Uh oh!

lightningRalf commented May 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants