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

refactor: Add ruff rules, improve type annotations, improve ci performance #3431

Merged
merged 101 commits into from
Jun 27, 2024

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Jun 5, 2024

Fixes: #3430

  • Adds most of the autofixable rules
  • Tweak pyproject.toml
  • Made some manual fixes, due to comment placement blocking the autofix
  • Resolved 2 issues relating to win32 and pyright
  • Rerun lint, format, schemagen

Stats from the first round of rules

ruff check . --statistics
Found 313 errors, 293 fixable [*].

Details
91      UP032   [*] Use f-string instead of `format` call
62      EM101   [*] Exception must not use a string literal, assign to variable first
28      EM103   [*] Exception must not use a `.format()` string directly, assign to variable first
24      EM102   [*] Exception must not use an f-string literal, assign to variable first
19      UP008   [*] Use `super()` instead of `super(__class__, self)`
11      RUF002  [ ] Docstring contains ambiguous `` (EN DASH). Did you mean `-` (HYPHEN-MINUS)?
10      RUF100  [*] Unused `noqa` directive (non-enabled: `C901`)
 8      PIE790  [*] Unnecessary `pass` statement
 8      UP030   [*] Use implicit references for positional format fields
 6      SIM102  [ ] Use a single `if` statement instead of nested `if` statements
 6      SIM118  [*] Use `key in dict` instead of `key in dict.keys()`
 5      SIM108  [*] Use ternary operator `cls = matches[0] if matches else default_class` instead of `if`-`else`-block
 4      SIM117  [ ] Use a single `with` statement with multiple contexts instead of nested `with` statements
 4      SIM300  [*] Yoda conditions are discouraged, use `Version("1.0.0") <= PANDAS_VERSION` instead
 4      RUF005  [*] Consider `(*scope, group_index)` instead of concatenation
 3      SIM910  [*] Use `channel_to_name.get(type_)` instead of `channel_to_name.get(type_, None)`
 3      UP015   [*] Unnecessary open mode parameters
 3      RUF015  [*] Prefer `next(iter(dct["datasets"].values()))` over single element slice
 2      PIE800  [*] Unnecessary spread `**`
 2      PIE810  [*] Call `startswith` once with a `tuple`
 2      SIM105  [ ] Use `contextlib.suppress(AttributeError)` instead of `try`-`except`-`pass`
 2      UP004   [*] Class `ParameterExpression` inherits from `object`
 2      UP027   [*] Replace unpacked list comprehension with a generator expression
 1      PIE807  [*] Prefer `dict` over useless lambda
 1      UP028   [*] Replace `yield` over `for` loop with `yield from`
 1      UP031   [*] Use format specifiers instead of percent format
 1      RUF010  [*] Use explicit conversion flag

Tasks

  • Find source of RUF002 characters

    Docstring contains ambiguous (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCE...

    Docstring contains ambiguous (EN DASH). Did you mean - (HYPHEN-MINUS)?

  • flake8-use-pathlib (PTH) (all manual)
  • Preview rules
  • Resolve cases of comments being lost
    • Seems to occur when a comment is nested within an elif/else, but is collapsed after linting
  • flake8-type-checking (TCH)/FA100/pyupgrade (UP) manual component
    • For many of these fixes, you first need a from __future__ import annotations declaration at the top of each file.
    • I want to be sure all the maintainers (particularly @binste) understand the impact this can have on any code that relies on typing constructs storing class objects - which would then be "stringified".
    • It would still be a huge positive in simplifying imports, but there may be scenarios where the new behaviour is a source of unexpected issues
    • See PEP 563
    • Also PEP 649 which is due for python=3.14
      suggesting requires further discussion
  • Library-specific (mostly manual)

Future Rule Considerations

Across the 4 `PL.` categories, there are over 50 rules, with a mix of fixable, preview.
Tried to be picky with what is added, because adding even 1 of the groups would generate a lot of manual fixes.
…_selection`

`param_kwds` in `_selection` triggered `PLR6201`. Instead rewrote as a dictcomp.
`flake8-use-pathlib` rules all require manual fixes.
> Found 70 errors.

<details>
<summary>Details</summary>

```md
20      PTH118  `os.path.join()` should be replaced by `Path` with `/` operator
12      PTH120  `os.path.dirname()` should be replaced by `Path.parent`
11      PTH100  `os.path.abspath()` should be replaced by `Path.resolve()`
11      PTH123  `open()` should be replaced by `Path.open()`
 7      PTH110  `os.path.exists()` should be replaced by `Path.exists()`
 6      PTH107  `os.remove()` should be replaced by `Path.unlink()`
 3      PTH103  `os.makedirs()` should be replaced by `Path.mkdir(parents=True)`
```
```log
Extension error (sphinxext.altairgallery): Handler <function main at 0x7f0873471ab0> for event 'builder-inited' threw an exception (exception: unsupported operand type(s) for +: 'PosixPath' and 'str')
```
Intended to discard the changes, but accidentally pushed
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.

This PR grew quite big and I struggle to review it fully so I just did some spotchecks. I think that's fine given the test coverage and linters. I'm sometimes guilty of the same crime and let PRs get quite big... ;) That being said, I really appreciate all the work you put into this, I'm aware that it was probably challenging to keep all PRs in sync, and I think it's a great improvement to the quality of the code base.

I've only added some remarks which should be quick to resolve. The most important one might be the naming of Optional which worries me a bit and I hope we can find a better name there. After that, I'm ok to get this into the code base. Also ok with the __future__ import, thanks for the research on it.

.github/workflows/build.yml Outdated Show resolved Hide resolved
tools/generate_schema_wrapper.py Outdated Show resolved Hide resolved
@@ -741,6 +742,18 @@ def __repr__(self) -> str:


Undefined = UndefinedType()
T = TypeVar("T")
Optional: TypeAlias = Union[T, UndefinedType]
Copy link
Contributor

Choose a reason for hiding this comment

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

I do like the use of a TypeVar for this but I don't know if we should name it Optional as it already has a different meaning which is well known to users. If I read Optional in my IDE, I expect that I can pass None and I might write code for it. What do you think? I can't come up with a better name for it right now...

I really like the literals to reduce the code!

@dangotbanned
Copy link
Member Author

@binste I can't seem to be able to reply to this comment

I do like the use of a TypeVar for this but I don't know if we should name it Optional as it already has a different meaning which is well known to users. If I read Optional in my IDE, I expect that I can pass None and I might write code for it. What do you think? I can't come up with a better name for it right now...

I do agree with this sentiment, and when I first added the alias in f1d9dc2 I used the name _AltOptional for this exact reason.

By the time I added it to this branch, I'd changed my mind and added the reasoning to the commit message:

typing.Optional is deprecated and is no longer needed in altair. The new alias improves the readability of generated signatures, by reducing noise (repeating UndefinedType) and clearly grouping together the options for an argument.
https://typing.readthedocs.io/en/latest/spec/historical.html#union-and-optional

I'm not sure about all IDEs, but for VSCode the "dangling" docstring will show on hover.

Just spotted I need to update the example since https://github.com/microsoft/pylance-release/releases/tag/2024.6.100 added sphinx support.

image

I expect that I can pass None

That is fair. Although say for the example above, the arguments accepting None are already differentiated.
A user with a type-checker will see an error for None on aggregate, bandPosition - but not bin, field, header.

I am open to an alternative, but the semantics of this alias and typing.Optional are so similar that I felt we could reuse the name - as long as typing.Optional appeared nowhere in altair - to avoid any confusion.

I really like the literals to reduce the code!

Thank you. I was really suprised at how much more readable it made the signatures, especially those with multiple literals.

@binste
Copy link
Contributor

binste commented Jun 27, 2024

Valid points. I can't come up with a better name either so let's give it a try like this.

I'll merge this in to free up other PRs. Refinements such as updating examples can happen in a follow-up PR.

@binste binste merged commit 485eae5 into vega:main Jun 27, 2024
11 checks passed
@dangotbanned
Copy link
Member Author

Just seen this https://github.com/astral-sh/ruff/releases/tag/0.5.0

@binste thanks for reviewing & merging.

I'll continue in another PR as suggested

dangotbanned added a commit to dangotbanned/altair that referenced this pull request Jun 28, 2024
Fixes an issue identified in vega#3431 (comment) and addresses `typing.Optional`
@dangotbanned dangotbanned deleted the update-ruff-rules branch June 28, 2024 12:57
dangotbanned added a commit to dangotbanned/altair that referenced this pull request Jun 30, 2024
Fixes https://github.com/vega/altair/actions/runs/9733301918/job/26860053484?pr=3452

This was due to `ruff` updating around the same time as vega#3431 (comment)
The `SIM` rule that it  is remapped to is already part of our config
dangotbanned added a commit to dangotbanned/altair that referenced this pull request Jul 8, 2024
Following vega#3431 a number of these are now importable.
Additionally, I spotted the `encodings` parameter was annotated with `str`, but should be restricted to the constraints of `SingleDefUnitChannel_T`.
binste pushed a commit that referenced this pull request Jul 9, 2024
Following #3431 a number of these are now importable.
Additionally, I spotted the `encodings` parameter was annotated with `str`, but should be restricted to the constraints of `SingleDefUnitChannel_T`.
dangotbanned added a commit to dangotbanned/altair that referenced this pull request Aug 9, 2024
https://docs.astral.sh/ruff/settings/#lint_mccabe_max-complexity
Previously 18, the default is 10.

As this rule was not in `[tool.ruff.lint.select]`, it was not used.

Setting such a high limit hides opportunities to simplify large, complex functions.
Many cases were resolved/improved in vega#3431.
The remainder can use the *# noqa: C901* comment to serve as a TODO
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add additional ruff rules to pyproject.toml
4 participants