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

[pycodestyle] Do not trigger E3 rules on defs following a function/method with a dummy body #10704

Merged
merged 8 commits into from Apr 15, 2024

Conversation

hoel-bagard
Copy link
Contributor

Summary

Closes #10211

This PR keeps track of functions with a dummy body, and does not trigger E301, E302 or E306 on defs following functions with a dummy body.

Test Plan

The code snippets from the issue have been added to the fixture.
On that note, so far the test cases for the E3 rules have been grouped by rule, since it made things easier when checking the results by hand. However it now makes it harder to review the snapshots (here nothing really changed, only line numbers), so I can add the new tests at the end of the fixture file if that makes things easier.

Copy link
Contributor

github-actions bot commented Apr 1, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -5 violations, +0 -0 fixes in 1 projects; 43 projects unchanged)

zulip/zulip (+0 -5 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- scripts/lib/zulip_tools.py:601:1: E302 [*] Expected 2 blank lines, found 0
- zerver/decorator.py:556:1: E302 [*] Expected 2 blank lines, found 0
- zerver/lib/validator.py:268:1: E302 [*] Expected 2 blank lines, found 0
- zproject/config.py:33:1: E302 [*] Expected 2 blank lines, found 0
- zproject/config.py:56:1: E302 [*] Expected 2 blank lines, found 0

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
E302 5 0 5 0 0

@hoel-bagard hoel-bagard marked this pull request as ready for review April 1, 2024 12:42
@MichaReiser MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Apr 8, 2024
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thank you.

The ecosystem checks look good to me. I recommend adding a few more tests (see inline comments) but we can then land your change.

Comment on lines 1037 to 1039
&& !(state.follows.is_any_def() && line.last_token != TokenKind::Colon)
// Allow a function/method to follow a function/method with a dummy body.
&& !matches!(state.follows, Follows::DummyDef)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It might be worth extracting this check into a small helper function to avoid repeating it three times (and it gives as the opportunity to give it a meaningful name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 6751ef1.

@MichaReiser
Copy link
Member

Thanks for addressing the feedback. I overlooked this last time but the rule should continue to error if there's only one blank line between two functions (it should either be 0 or 2, but never 1). I found this in the ecosystem checks.

@overload
def components(models: dict[str, Model], wrap_script: bool = ..., # type: ignore[overload-overlap] # XXX: mypy bug
    wrap_plot_info: Literal[True] = ..., theme: ThemeLike = ...) -> tuple[str, dict[str, str]]: ...
@overload
def components(models: dict[str, Model], wrap_script: bool = ..., wrap_plot_info: Literal[False] = ...,
    theme: ThemeLike = ...) -> tuple[str, dict[str, RenderRoot]]: ...

def components(models: Model | Sequence[Model] | dict[str, Model], wrap_script: bool = True,
               wrap_plot_info: bool = True, theme: ThemeLike = None) -> tuple[str, Any]:
    ''' Return HTML components to embed a Bokeh plot. The data for the plot is
    stored directly in the returned HTML.

    An example can be found in examples/embed/embed_multiple.py'''

This should error because there's only a single blank line between the implementation and the last @overload declaration.

@hoel-bagard
Copy link
Contributor Author

I didn't think about that, I've fixed it in ad7e466.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks

@MichaReiser MichaReiser merged commit 670d66f into astral-sh:main Apr 15, 2024
17 checks passed
@hoel-bagard hoel-bagard deleted the fix-10211 branch April 15, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Harmonize E301 check with format in --preview mode?
2 participants