use `ruff` as linter #3008

mattijn · 2023-04-01T07:50:47Z

This PR add ruff as linter over flake8. This in order to work towards a complete pyproject.toml-based build, see corresponding PR #3007.

I've moved the flake8 configuration to the corresponding ruff section in the pyproject.toml file. I tried to synchronize the same tests for now. But still had to apply some fixes in the code to get in sync

This PR include code fixes for B028, B904, B006 and B018.
I've excluded TID252 (relative imports) and B905 (valid from python>3.10).
E203, E266 and W503 checks are not yet available in ruff, see Implement remaining pycodestyle rules astral-sh/ruff#2402, so these cannot be excluded yet (as they were for flake8).

Ready for review.

No explicit `stacklevel` keyword argument found

mattijn · 2023-04-01T19:03:22Z

I cannot assign you as reviewer @binste, but would you be willing to check this?

binste

Sure! Always happy to review.

Thank you for tackling this and also for going through the rules to see what is available! I'm amazed by the speed of ruff and how many rules are already implemented... Maybe we can even use it as a replacement for black at one point: astral-sh/ruff#1904

Looks mostly good to me. Added some smaller code suggestions. Could you also replace the mention of flake8 in contributing.md?

.github/workflows/lint.yml

Makefile

binste · 2023-04-02T07:07:03Z

altair/utils/schemapi.py

@@ -564,11 +564,17 @@ def to_dict(self, validate=True, ignore=None, context=None):
            try:
                self.validate(result)
            except jsonschema.ValidationError as err:
-                raise SchemaValidationError(self, err)
+                raise SchemaValidationError(self, err) from err


Suggested change

raise SchemaValidationError(self, err) from err

# We do not raise `from err` as else the resulting

# traceback is very long as it contains part

# of the Vega-Lite schema. It would also first

# show the less helpful ValidationError instead of

# the more user friendly SchemaValidationError

raise SchemaValidationError(self, err) from None

I've modified your suggestion a little bit to raise from None instead of raise # noqa: B904. I'm not entirely sure if that leads to the same behavior. Do you have an example how you checked this?

Double-checked and that works as well. It gives the same traceback as in master which is good. I tested it with:

import altair as alt from vega_datasets import data ( alt.Chart(data.barley()) .mark_bar() .encode( x=alt.X("variety", unknown=2), y=alt.Y("sum(yield)", stack="asdf"), ) )

binste · 2023-04-02T07:07:35Z

tools/schemapi/schemapi.py

@@ -562,11 +562,17 @@ def to_dict(self, validate=True, ignore=None, context=None):
            try:
                self.validate(result)
            except jsonschema.ValidationError as err:
-                raise SchemaValidationError(self, err)
+                raise SchemaValidationError(self, err) from err


Suggested change

raise SchemaValidationError(self, err) from err

# We do not raise `from err` as else the resulting

# traceback is very long as it contains part

# of the Vega-Lite schema. It would also first

# show the less helpful ValidationError instead of

# the more user friendly SchemaValidationError

raise SchemaValidationError(self, err) from None

Same change as above, raise from None instead of rase # noqa: B904

altair/utils/schemapi.py

tools/schemapi/schemapi.py

setup.cfg

mattijn

Thanks for the review @binste. I've one inline question remaining.

.github/workflows/lint.yml

Makefile

tools/schemapi/schemapi.py

setup.cfg

altair/utils/schemapi.py

mattijn · 2023-04-02T19:59:13Z

Hm, it seems there are some sync issues with VSCode replies. Let me respond using the replies on github website.

mattijn · 2023-04-02T20:27:24Z

Maybe we can even use it as a replacement for black at one point: astral-sh/ruff#1904

Hm, didn't realize this, but I think we can remove black for format suggestions already? The link you refer to is on ruff autoformat, but I don't think we have black autoformat enabled?

In that case black --check . should raise the same as ruff check ., agree? Or I miss something here?

binste · 2023-04-03T17:38:39Z

I didn't find any options for ruff to enable checks on code formatting. Also tested it on some code where black correctly shows an error but ruff does not. The FAQ state that "As a project, Ruff is designed to be used alongside Black and, as such, will defer implementing stylistic lint rules that are obviated by autoformatting." So I'd assume that the checking comes hand-in-hand with the autoformat capability and we need to wait for that.

binste

Looks good! This inspired me to start using ruff at work as well.

mattijn · 2023-04-04T19:15:37Z

Thanks @binste! Great that the insights here is more useful than this PR alone👍.
So we will have to wait until ruff has autoformat support before we can switch. I'll open an issue for this to keep track on it. Merging this, thanks for the review!

charliermarsh · 2023-04-04T20:15:43Z

Sorry for the drive-by comment but I just saw this is in Ruff's dependency graph and as a long-time Altair user I can't help but mention how cool it is to be able to support the project. Let me know if you ever run into issues with Ruff or have questions on usage etc.

joelostblom · 2023-04-04T20:22:40Z

That's great, thank you for taking the time to comment and offering your support @charliermarsh !

mattijn added 15 commits April 1, 2023 09:22

update pyproject.toml

f153a86

update requirements_dev.txt

6a1863c

update makefile

0e544f4

update lint.yml

b800d98

remove setup.cfg

2e955bf

update files to ruff: noqa

7d8084b

update changes.rst

0a7748f

ruff auto-fixes

ed41732

fixes B028

20ad75b

No explicit `stacklevel` keyword argument found

fix B028

036e332

fixes B904

dafedce

fixes B006

12b5033

ignore B905 (python>3.10 only)

545dfd4

fixes B018

541fcc6

apply black

4240ee3

mattijn changed the title ~~wip To ruff~~ use ruff as linter Apr 1, 2023

mattijn marked this pull request as ready for review April 1, 2023 19:02

binste requested changes Apr 2, 2023

View reviewed changes

code-review changes

32abf45

mattijn commented Apr 2, 2023

View reviewed changes

.github/workflows/lint.yml Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

tools/schemapi/schemapi.py Show resolved Hide resolved

setup.cfg Show resolved Hide resolved

altair/utils/schemapi.py Show resolved Hide resolved

code-review changes

947ee0c

binste approved these changes Apr 3, 2023

View reviewed changes

mattijn merged commit d7c0b22 into vega:master Apr 4, 2023

mattijn mentioned this pull request Apr 4, 2023

Black to ruff #3011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use `ruff` as linter #3008

use `ruff` as linter #3008

mattijn commented Apr 1, 2023 •

edited

Loading

mattijn commented Apr 1, 2023

binste left a comment

binste Apr 2, 2023 •

edited by mattijn

Loading

mattijn Apr 2, 2023 •

edited

Loading

binste Apr 3, 2023

binste Apr 2, 2023 •

edited by mattijn

Loading

mattijn Apr 2, 2023

mattijn left a comment

mattijn commented Apr 2, 2023

mattijn commented Apr 2, 2023 •

edited

Loading

binste commented Apr 3, 2023

binste left a comment

mattijn commented Apr 4, 2023

charliermarsh commented Apr 4, 2023

joelostblom commented Apr 4, 2023

-                raise SchemaValidationError(self, err) from err
+                # We do not raise `from err` as else the resulting
+                # traceback is very long as it contains part
+                # of the Vega-Lite schema. It would also first
+                # show the less helpful ValidationError instead of
+                # the more user friendly SchemaValidationError
+                raise SchemaValidationError(self, err) from None

use ruff as linter #3008

use ruff as linter #3008

Conversation

mattijn commented Apr 1, 2023 • edited Loading

mattijn commented Apr 1, 2023

binste left a comment

Choose a reason for hiding this comment

binste Apr 2, 2023 • edited by mattijn Loading

Choose a reason for hiding this comment

mattijn Apr 2, 2023 • edited Loading

Choose a reason for hiding this comment

binste Apr 3, 2023

Choose a reason for hiding this comment

binste Apr 2, 2023 • edited by mattijn Loading

Choose a reason for hiding this comment

mattijn Apr 2, 2023

Choose a reason for hiding this comment

mattijn left a comment

Choose a reason for hiding this comment

mattijn commented Apr 2, 2023

mattijn commented Apr 2, 2023 • edited Loading

binste commented Apr 3, 2023

binste left a comment

Choose a reason for hiding this comment

mattijn commented Apr 4, 2023

charliermarsh commented Apr 4, 2023

joelostblom commented Apr 4, 2023

use `ruff` as linter #3008

use `ruff` as linter #3008

mattijn commented Apr 1, 2023 •

edited

Loading

binste Apr 2, 2023 •

edited by mattijn

Loading

mattijn Apr 2, 2023 •

edited

Loading

binste Apr 2, 2023 •

edited by mattijn

Loading

mattijn commented Apr 2, 2023 •

edited

Loading