FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-42978: Improve error message when 'self' is missing from the method definition by pablogsal · Pull Request #24272 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-42978: Improve error message when 'self' is missing from the method definition - #24272

Closed
pablogsal wants to merge 5 commits into
python:masterfrom
pablogsal:better_error
ConversationCommits (5)ChecksFiles changed
Closed

bpo-42978: Improve error message when 'self' is missing from the method definition#24272
pablogsal wants to merge 5 commits into
python:masterfrom
pablogsal:better_error

Conversation

pablogsal commented Jan 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

⚠️ I just opened the draft to discuss the approach,⚠️

https://bugs.python.org/issue42978

abunuwas commented Jan 20, 2021

Copy link
Copy Markdown

As far as I know it's not compulsory to name the object parameter self isn't it? If somebody's calling it this or that this error message could get confusing.
Also in cases where the developer has genuinely passed more arguments than allowed, inquiring about whether self is not necessarily helpful?

pablogsal commented Jan 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

As far as I know it's not compulsory to name the object parameter self isn't it?

This is so extended that everyone will recognize the problem immediately.

Also in cases where the developer has genuinely passed more arguments than allowed, inquiring about whether self is not necessarily helpful?

That is why the message is in the form of a question: it hints something to the developer to check, is not asserting that that is the error.

Additionally, this error message has been battle-tested by pypy.

pablogsal force-pushed the better_error branch 5 times, most recently from a646cc2 to cb737e0 Compare January 20, 2021 17:41
serhiy-storchaka reviewed Jan 20, 2021
View reviewed changes
Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated

static void improve_missing_self_error(PyThreadState* tstate, PyCodeObject* co, Py_ssize_t nargs) {

if (nargs + 1 != co->co_argcount) {

serhiy-storchaka Jan 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What about positional-only parameters and var-positional parameter?

pablogsal Jan 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I left that for future PRs, I wanted to get the basic logic first and then improve upon. But is just mirroring the logic in too_many_positional().

serhiy-storchaka Jan 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is the check !(co->co_flags & CO_VARARGS) befory calling too_many_positional().

pablogsal closed this Jan 21, 2021
serhiy-storchaka reviewed Jan 21, 2021
View reviewed changes
Comment thread Python/ceval.c
if (co->co_argcount > 0 && co->co_varnames && PyTuple_GET_SIZE(co->co_varnames) > 0) {
PyObject* first_arg = PyTuple_GET_ITEM(co->co_varnames, 0);
if (_PyUnicode_EqualToASCIIString(first_arg, "self")) {
PyErr_Clear();

serhiy-storchaka Jan 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It is not needed.

Comment thread Python/ceval.c Outdated

static void improve_missing_self_error(PyThreadState* tstate, PyCodeObject* co, Py_ssize_t nargs) {

if (nargs + 1 != co->co_argcount) {

serhiy-storchaka Jan 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is the check !(co->co_flags & CO_VARARGS) befory calling too_many_positional().

pablogsal commented Jan 21, 2021

Copy link
Copy Markdown
Member Author

Thanks a lot for the thorough review (I missed working with you on PRs 😄 ), @serhiy-storchaka! In the end, it seems that the general consensus is that this is not worth the downsides so I closed the PR.

pablogsal deleted the better_error branch May 19, 2021 18:59
wookie184 mentioned this pull request Jul 2, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

serhiy-storchaka serhiy-storchaka left review comments

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL