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

[flake8-bugbear] Treat raise NotImplemented-only bodies as stub functions #10990

Merged
merged 9 commits into from
Apr 17, 2024

Conversation

Philipp-Thiel
Copy link
Contributor

Summary

As discussed in #10083 (comment), stubs detection now also covers the case where the function body raises NotImplementedError and does nothing else.

Test Plan

Tests for the relevant cases were added in B006_8.py

Copy link
Contributor

github-actions bot commented Apr 17, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh self-assigned this Apr 17, 2024
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Thanks!

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Apr 17, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) April 17, 2024 13:59
}) => exception.as_ref().is_some_and(|exc| {
semantic
.resolve_builtin_symbol(map_callable(exc))
.is_some_and(|name| matches!(name, "NotImplementedError" | "NotImplemented"))
Copy link
Member

Choose a reason for hiding this comment

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

Changed this to use the semantic model, and allow raise NotImplementedError (without argument) and raise NotImplemented.

Choose a reason for hiding this comment

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

Why do you allow raise NotImplemented? NotImplemented is not an exception. It should never be raised. NotImplemented can be returned by comparison operators.

Copy link
Member

Choose a reason for hiding this comment

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

We have separate rules to guard against raise NotImplemented. But if the entire function is raise NotImplemented, it's almost certainly intended to be raise NotImplementedError, right?

Choose a reason for hiding this comment

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

I see, fair enough!

@charliermarsh charliermarsh changed the title B006 stubs extension to include NotImplementedError [flake8-bugbear] Treat raise NotImplemented-only bodies as stub functions Apr 17, 2024
@charliermarsh charliermarsh disabled auto-merge April 17, 2024 13:59
@charliermarsh charliermarsh enabled auto-merge (squash) April 17, 2024 14:00
@charliermarsh charliermarsh merged commit 2971655 into astral-sh:main Apr 17, 2024
17 checks passed


def quux(a: list = []):
raise NotImplemented

Choose a reason for hiding this comment

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

What's the purpose of this example?

Copy link
Member

Choose a reason for hiding this comment

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

To verify that we treat raise NotImplemented like raise NotImplementedError. (Just to be clear, these aren't examples, they're tests.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants