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

use ruff as linter #3008

Merged
merged 17 commits into from
Apr 4, 2023
Merged

use ruff as linter #3008

merged 17 commits into from
Apr 4, 2023

Conversation

mattijn
Copy link
Contributor

@mattijn mattijn commented Apr 1, 2023

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.

@mattijn mattijn changed the title wip To ruff use ruff as linter Apr 1, 2023
@mattijn mattijn marked this pull request as ready for review April 1, 2023 19:02
@mattijn
Copy link
Contributor Author

mattijn commented Apr 1, 2023

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

Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
.github/workflows/lint.yml Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@@ -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
Copy link
Contributor

@binste binste Apr 2, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mattijn mattijn Apr 2, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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"),
    )
)

@@ -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
Copy link
Contributor

@binste binste Apr 2, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

altair/utils/schemapi.py Show resolved Hide resolved
tools/schemapi/schemapi.py Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
Copy link
Contributor Author

@mattijn mattijn 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 the review @binste. I've one inline question remaining.

.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
@mattijn
Copy link
Contributor Author

mattijn commented Apr 2, 2023

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

@mattijn
Copy link
Contributor Author

mattijn commented Apr 2, 2023

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
Copy link
Contributor

binste commented Apr 3, 2023

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.

Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

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

@mattijn
Copy link
Contributor Author

mattijn commented Apr 4, 2023

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!

@mattijn mattijn merged commit d7c0b22 into vega:master Apr 4, 2023
@mattijn mattijn mentioned this pull request Apr 4, 2023
@charliermarsh
Copy link

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
Copy link
Contributor

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

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.

None yet

4 participants