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

Jump to bottom

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

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

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.

@kaushikcfd kaushikcfd requested a review from markshannon as a code owner Oct 6, 2022
@bedevere-bot
Copy link

bedevere-bot commented Oct 6, 2022

Most changes to Python require a NEWS entry.

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

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Oct 6, 2022 •
edited

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

bedevere-bot commented Oct 6, 2022

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 Oct 6, 2022
@bedevere-bot
Copy link

bedevere-bot commented Oct 6, 2022

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 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

@JelleZijlstra JelleZijlstra Oct 7, 2022

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

@kaushikcfd kaushikcfd Oct 9, 2022

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

bedevere-bot commented Oct 7, 2022

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

bedevere-bot commented Oct 9, 2022

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

bedevere-bot commented Oct 9, 2022

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

bedevere-bot commented Oct 9, 2022

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

bedevere-bot commented Oct 9, 2022

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

bedevere-bot commented Oct 9, 2022

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 Oct 9, 2022
@bedevere-bot
Copy link

bedevere-bot commented Oct 9, 2022

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

JelleZijlstra commented Oct 9, 2022

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

kaushikcfd commented Oct 9, 2022

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 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
Member

@JelleZijlstra JelleZijlstra Oct 14, 2022

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

@kaushikcfd kaushikcfd Oct 15, 2022

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

kaushikcfd commented Oct 15, 2022

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

sobolevn commented Oct 15, 2022

Retriggering CI, it is indeed unrelated.

@kaushikcfd
Copy link
Contributor Author

kaushikcfd commented Oct 15, 2022

Retriggering CI, it is indeed unrelated.

Thanks, yep that was it.

@kaushikcfd
Copy link
Contributor Author

kaushikcfd commented Nov 4, 2022

Soft ping.

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

@JelleZijlstra JelleZijlstra Nov 5, 2022

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

@kaushikcfd kaushikcfd Nov 5, 2022

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
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra

Assignees

@JelleZijlstra JelleZijlstra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kaushikcfd @bedevere-bot @JelleZijlstra @sobolevn