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

gh-99139: Improve NameError error suggestion for instances #99140

Merged
merged 7 commits into from
Nov 6, 2022

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Nov 5, 2022

Copy link
Member

@ammaraskar ammaraskar left a comment

Choose a reason for hiding this comment

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

Looks great :)

Copy link
Member

@isidentical isidentical left a comment

Choose a reason for hiding this comment

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

Looks really helpful, thanks a lot @pablogsal!

@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 5, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit b1546e5 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 5, 2022
pablogsal and others added 3 commits November 6, 2022 01:54
Co-authored-by: Ammar Askar <[email protected]>
Co-authored-by: Batuhan Taskaya <[email protected]>
Co-authored-by: Batuhan Taskaya <[email protected]>
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 6, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 4925719 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 6, 2022
Co-authored-by: Kumar Aditya <[email protected]>
@pablogsal pablogsal merged commit 99e2e60 into python:main Nov 6, 2022
@pablogsal pablogsal deleted the better_attr branch November 6, 2022 13:52
if (!locals) {
goto error;
}
PyObject* self = PyDict_GetItem(locals, &_Py_ID(self)); /* borrowed */
Copy link
Member

Choose a reason for hiding this comment

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

Do not use PyDict_GetItem. It is broken by design.

Copy link
Member Author

Choose a reason for hiding this comment

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

What should we use instead for both?

Copy link
Member

Choose a reason for hiding this comment

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

PyDict_GetItemWithError and PyObject_GetAttr if limited by public API. It is suggested in the documentation. But _PyObject_LookupAttr is more convenient in the latter case.

goto error;
}

if (PyObject_HasAttr(self, name)) {
Copy link
Member

Choose a reason for hiding this comment

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

Do not use PyObject_HasAttr. It is broken by design.

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.

6 participants