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

fix: produce standard Python repr style #183

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Sep 30, 2024

Promised follow to #175. Now the error messages produce standard output like Python, such as Field 'project.thing' got 'value'. By matching the built-in repr style, we can now use repr directly, which fixes the display when printout out a non-string, instead of things like "True", "13", "{}", etc. The confusing wrong type, expected string (got "2") messages are finally better. :)

As usual, while going over this, fixed minor things. In this case, an extra , in a error message.

self.config_error(msg, key="project.readme.content-type")
return None
else:
msg = (
f'Field "project.readme" has an invalid type, expecting either, '
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the removed extra comma

@henryiii henryiii merged commit c26b406 into pypa:main Sep 30, 2024
24 checks passed
@henryiii henryiii deleted the henryiii/fix/repr branch September 30, 2024 21:51
@dnicolodi
Copy link
Contributor

I'm not trilled by this change. I think that double quotes look much nicer as delimiters (and incidentally, we use double quotes for strings in the codebase now, so someone must think likewise). But, most importantly, this requires the users of pyproject-metadata to adjust the quoting style in their error messages to be consistent.

The main reason for doing this, is indeed to produce error messages embedding values that are not strings. However, I argue that in these cases, the value is not the important information, but rather the type, thus these error messages should be reworded to provide the type, not the value. wrong type, expected string (got "2") and wrong type, expected string (got 2) are different only if the reader knows that in this contest strings are quoted, otherwise they are identical. However, wrong type, expected string (got int)` is fairly straightforward to interpret.

However, if we are going in the direction of integrating pyproject-metadata into packaging, maybe the error messages format should be made to match the one used in packaging (assuming packaging uses a consistent formatting for error message). What does packaging do?

@henryiii
Copy link
Collaborator Author

henryiii commented Oct 1, 2024

Python itself uses single quotes as delimiters. So the built-in error message will all be formatted like this:

>>> int("x")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: invalid literal for int() with base 10: 'x'

As is packaging (and most other packages):

>>> packaging.version.Version("x")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/henryschreiner/git/software/python-pyproject-metadata/.venv/lib/python3.12/site-packages/packaging/version.py", line 202, in __init__
    raise InvalidVersion(f"Invalid version: '{version}'")
packaging.version.InvalidVersion: Invalid version: 'x'

Double quotes for strings makes this easier, since the single quotes can be embedded into the string naturally without escaping. Though when printing a variable, you can just use repr. I'd have written the above as f"Invalid version: {version!r}".

I do think printing the type for that specific case is a good idea, though.

@henryiii
Copy link
Collaborator Author

henryiii commented Oct 1, 2024

Showing the type isn't always better, I think. This message:

Field 'project.keywords' has an invalid type, expecting a list of strings (got [1])

Which is pretty clear, then would be:

Field 'project.keywords' has an invalid type, expecting a list of strings (got list)

I'll see if we can be more clever, though.

FYI, I'm not stuck on the project.x part of the message having single quotes. Maybe field names shouldn't match the repr values. There's some precedent for this; the path strings in the official error messages are double-quoted. So you'd get this:

Field "project.keywords" has an invalid type, expecting a list of strings (got 'some string')

Thoughts?

@henryiii
Copy link
Collaborator Author

henryiii commented Oct 2, 2024

Also backticks are an option (from @flying-sheep on Discord).

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.

2 participants