8000 gh-98284: better error message for undefined abstractmethod by kaushikcfd · Pull Request #97971 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-98284: better error message for undefined abstractmethod #97971

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 5, 2022
Merged

gh-98284: better error message for undefined abstractmethod #97971

merged 5 commits into from
Nov 5, 2022

Conversation

kaushikcfd
Copy link
Contributor
@kaushikcfd kaushikcfd commented Oct 6, 2022 •
edited by bedevere-bot
Loading

Error message before:

TypeError: Can't instantiate abstract class IndexNode with abstract method name

Error message after this patch:

TypeError: Can't instantiate abstract class IndexNode with abstract method 'name'

Given that the style-guide requires method names to be in lowercase, the quotation marks help in differentiating the sentence from the method's name.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost
Copy link
ghost commented Oct 6, 2022 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@kaushikcfd kaushikcfd requested a review from ericvsmith as a code owner October 6, 2022 18:10
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

JelleZijlstra
JelleZijlstra reviewed Oct 7, 2022
View reviewed changes
Lib/test/test_abc.py Outdated
@@ -165,7 +165,7 @@ def method_one(self):
@abc.abstractmethod
def method_two(self):
pass
msg = r"class C without an implementation for abstract methods method_one, method_two"
msg = r"class C without an implementation for abstract methods 'method_one, method_two'"
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect, we should quote each method individually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done!

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.
8000

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@kaushikcfd kaushikcfd requested review from JelleZijlstra and removed request for ericvsmith and markshannon October 9, 2022 16:10
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@JelleZijlstra
Copy link
Member

You do need a NEWS entry as the good knight Bedevere keeps telling you.

Also, avoid force-pushing; it makes review harder. We'll squash-merge anywhere when we merge a PR.

@kaushikcfd
Copy link
Contributor Author

You do need a NEWS entry as the good knight Bedevere keeps telling you.

Done!

Also, avoid force-pushing;

Noted, thanks!

JelleZijlstra
JelleZijlstra reviewed Oct 14, 2022
View reviewed changes
Misc/NEWS.d/next/Library/2022-10-09-18-08-08.gh-issue-97971.SaVHTd.rst Outdated
@@ -0,0 +1,3 @@
Improved :class:`TypeError` message for undefined abstract methods of a
Copy link
8000
Member

Choose a reason for hiding this comment

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

The NEWS entry must refer to an issue, not a PR. You'll have to create an issue and rename the file to have your issue number in the file 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.

Thanks, see 4073b1f.

@kaushikcfd kaushikcfd mentioned this pull request Oct 15, 2022
Closed
@kaushikcfd kaushikcfd changed the title [abc] better error message for undefined abstractmethod gh-98284: better error message for undefined abstractmethod Oct 15, 2022
@kaushikcfd
Copy link
Contributor Author

The CI failure is (most likely) unrelated:

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\a\\cpython\\cpython\\build\\test_python_2628�\\test_python_worker_1152�'

@sobolevn sobolevn closed this Oct 15, 2022
@sobolevn sobolevn reopened this Oct 15, 2022
@sobolevn
Copy link
Member

Retriggering CI, it is indeed unrelated.

@kaushikcfd
Copy link
Contributor Author

Retriggering CI, it is indeed unrelated.

Thanks, yep that was it.
A3E2

@kaushikcfd
Copy link
Contributor Author

Soft ping.

JelleZijlstra
JelleZijlstra reviewed Nov 5, 2022
View reviewed changes
Objects/typeobject.c
type->tp_name,
method_count > 1 ? "s" : "",
joined);
Py_DECREF(joined);
Py_DECREF(comma_w_quotes_sep);
Copy link
Member

Choose a reason for hiding this comment

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

You also need to DECREF this in the two return NULL branches above.

Come to think of it, the second of those already leaks joined. It needs a Py_DECREF(joined).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aah, yep! Fixed in 6ae052e.

@JelleZijlstra JelleZijlstra self-assigned this Nov 5, 2022
@JelleZijlstra JelleZijlstra merged commit 67ade40 into python:main Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

Assignees

@JelleZijlstra JelleZijlstra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kaushikcfd @bedevere-bot @JelleZijlstra @sobolevn
0