Skip to content

gh-117557: Improve error message for 'C' format if given string is not 1 char long #117558

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

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Apr 5, 2024

…ng for int(accept={str}) Argument Clinic converter
@erlend-aasland

This comment was marked as outdated.

@erlend-aasland erlend-aasland changed the title gh-117557: Raise ValueError iso. TypeError if str length is wrong for int(accept={str}) Argument Clinic converter gh-117557: Improve the error message if str length is != 1 for int(accept={str}) Argument Clinic converter Apr 5, 2024
@erlend-aasland erlend-aasland marked this pull request as ready for review April 5, 2024 11:08
@erlend-aasland erlend-aasland requested a review from a team as a code owner April 5, 2024 11:08
@erlend-aasland erlend-aasland changed the title gh-117557: Improve the error message if str length is != 1 for int(accept={str}) Argument Clinic converter gh-117557: Argument Clinic: improve int(accept={str}) converter error messages if given str is not 1 char long Apr 5, 2024
@erlend-aasland erlend-aasland changed the title gh-117557: Argument Clinic: improve int(accept={str}) converter error messages if given str is not 1 char long gh-117557: Argument Clinic: improve int(accept={str}) converter error message if given str is not 1 char long Apr 5, 2024
Copy link
Contributor

@ronaldoussoren ronaldoussoren left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Is it really better than the current error message?

In any case, you cannot just change the error message in Argument Clinic. It should match the error message in PyArg_Parse.

There are other variants of the error message for the same error in ord, str.center, str and bytes formatting %c. If we change the error messages, it is better to consider all options and make them as consistent as possible.

@erlend-aasland
Copy link
Contributor Author

Is it really better than the current error message?

Yes; it is clearer and more accurate.

There are other variants of the error message for the same error in ord, str.center, str and bytes formatting %c. If we change the error messages, it is better to consider all options and make them as consistent as possible.

They represent different APIs and require different error messages. Aligning them with this error message does not make sense to me.

@erlend-aasland
Copy link
Contributor Author

In any case, you cannot just change the error message in Argument Clinic. It should match the error message in PyArg_Parse.

Yes, I think there is value in synchronising these.

- try to make the error message even clearer

- sync with PyArg_Parse
@erlend-aasland
Copy link
Contributor Author

I tried to address your concerns in fe02edd, Serhiy.

@erlend-aasland erlend-aasland changed the title gh-117557: Argument Clinic: improve int(accept={str}) converter error message if given str is not 1 char long gh-117557: Improve error message for 'C' format if given string is not 1 char long Apr 5, 2024
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.

Thank you, it is better now. But note that other error message (few lines above) requires "a unicode character", not "a string containing exactly one unicode character". The latter looks as a more clumsy formulation of the former.

Ideally, the error message should contain the length of the string if it is not 1. But the PyArg_Parse code has limited facilities, so it needs more work.

Please look at more general image. Look how the affected functions are documented. Should the documentation be updated?

Look also at the 'c' format unit (bytes objects with length 1). Does it need changes?

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Apr 8, 2024
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Apr 8, 2024
@erlend-aasland
Copy link
Contributor Author

Closed in favour of #117631.

@erlend-aasland erlend-aasland deleted the unicodedata/single-char-expected branch April 8, 2024 11:04
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.

3 participants