Skip to content

gh-133436: Correct error messages that incorrectly use "arguments" #133463

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

Closed
wants to merge 2 commits into from

Conversation

StanFromIreland
Copy link
Member

@StanFromIreland StanFromIreland commented May 5, 2025

| '*' ',' TYPE_COMMENT { RAISE_SYNTAX_ERROR("bare * has associated type comment") }
| '*' param a='=' { RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "var-positional argument cannot have default value") }
| '*' (param_no_default | ',') param_maybe_default* a='*' (param_no_default | ',') {
RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "* argument may appear only once") }
RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "* marker may appear only once") }
Copy link
Member Author

Choose a reason for hiding this comment

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

I liked @picnixz suggestion though there may be an even better word?

Copy link

@Kivooeo Kivooeo May 5, 2025

Choose a reason for hiding this comment

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

imo, "an asterisk may appear only once" is as clear as possible and will not cause any confusion in terms of terminology

Copy link
Member

Choose a reason for hiding this comment

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

Other error messages refer to / as just / and to * as just *. On other hand, * marker can be confused with bare *. So I think that it is better to use just * here.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

There are more:

    def f(/, *a, b): pass
          ^
SyntaxError: at least one argument must precede /
    def f(**a, b): pass
               ^
SyntaxError: arguments cannot follow var-keyword argument

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

More examples:

    def f(/, a, /, b): pass
          ^
SyntaxError: at least one argument must precede /
    def f(a, a): pass
             ^
SyntaxError: duplicate argument 'a' in function definition

| '*' ',' TYPE_COMMENT { RAISE_SYNTAX_ERROR("bare * has associated type comment") }
| '*' param a='=' { RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "var-positional argument cannot have default value") }
| '*' (param_no_default | ',') param_maybe_default* a='*' (param_no_default | ',') {
RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "* argument may appear only once") }
RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "* marker may appear only once") }
Copy link
Member

Choose a reason for hiding this comment

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

Other error messages refer to / as just / and to * as just *. On other hand, * marker can be confused with bare *. So I think that it is better to use just * here.

@picnixz
Copy link
Member

picnixz commented May 9, 2025

The rest is in #133382, but maybe we can do all in one PR? I suggested two because of semantics, but maybe it's better to do everything at once (in this case, let's use #133382 instead and close the other issue as well)

@serhiy-storchaka
Copy link
Member

I suggest to do all in one PR. I do not see semantic difference. There are other error messages that need corrections, see my examples.

@picnixz
Copy link
Member

picnixz commented May 9, 2025

I'll close this one and the corresponding issue. We'll work with one issue and one PR instead.

@picnixz picnixz closed this May 9, 2025
@StanFromIreland StanFromIreland deleted the marker-err branch May 9, 2025 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants