Skip to content

B024: don't warn on classes without methods #336

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

Merged
merged 3 commits into from
Jan 19, 2023

Conversation

jakkdl
Copy link
Contributor

@jakkdl jakkdl commented Jan 18, 2023

Fix another case in #278 - so now it won't warn on empty classes or any class without methods, esp since the fix proposed in the error message (consider adding @abstractmethod) is not useful in that case.
@Zac-HD @mikhail-melnik

@jakkdl jakkdl force-pushed the B024_empty_abstract_class branch from 86ef281 to 36d815a Compare January 18, 2023 10:30
@jakkdl
Copy link
Contributor Author

jakkdl commented Jan 18, 2023

The error message in the readme and the message being raised were very different, so synced them to match better, and updated the message to be "softer".

Copy link
Member

@Zac-HD Zac-HD 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 to me, thanks @jakkdl!

cc @cooperlees in case you want to review, or plan a release?

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Do we want to note the logic change / bug fix in the README?

@jakkdl jakkdl force-pushed the B024_empty_abstract_class branch from 36d815a to 506f785 Compare January 19, 2023 08:42
the error message, and changed the message in the README to match it
better.
@jakkdl jakkdl force-pushed the B024_empty_abstract_class branch from 506f785 to 8116fdb Compare January 19, 2023 08:43
@jakkdl
Copy link
Contributor Author

jakkdl commented Jan 19, 2023

Fixed review comments and rebased on top of main

Yup, much clearer.

Co-authored-by: Zac Hatfield-Dodds <[email protected]>
@jakkdl
Copy link
Contributor Author

jakkdl commented Jan 19, 2023

pre-commit.ci autofix

@cooperlees cooperlees merged commit 5d9d744 into PyCQA:main Jan 19, 2023
@jakkdl jakkdl deleted the B024_empty_abstract_class branch January 19, 2023 22:24
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.

3 participants