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-124835: tomllib.loads: Raise TypeError not AttributeError. Improve message #124587

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

hukkin
Copy link
Contributor

@hukkin hukkin commented Sep 26, 2024

This PR makes tomllib.loads(b"") throw

TypeError: Expected str object, not 'bool'

instead of

TypeError: a bytes-like object is required, not 'str'

Given an input type that doesn't have the replace attribute, it makes tomllib.loads raise the same new TypeError, instead of an AttributeError. Should this be a separate PR though?

The corresponding Tomli PR is hukkin/tomli#229
The issue report is hukkin/tomli#223
Pinging @encukou as requested here hukkin/tomli#223 (comment)

Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Two stances:

  • I'm ok with the change but I would let a regular TypeError be propagated instead (e.g., if someone is using a subclass os str for whatever reason).
  • I'm not ok with the change because of the "garbage in, garbage out" principle, though it's not really an in/out here. I think loads should only expect a string.

For the second point, we could do something like isinstance(s, str) but this would restrict the user to always use a string instance and not a UserString object for instance.

I don't know which one is better. But the fact that the message does not help is IMO an inconvenience so I like that you want to fix it. Now, how to fix it, is still something I'm pondering.

Lib/test/test_tomllib/test_error.py Show resolved Hide resolved
Lib/tomllib/_parser.py Outdated Show resolved Hide resolved
@picnixz
Copy link
Contributor

picnixz commented Sep 26, 2024

I suggest opening a CPython issue so that we can possibly discuss it. In addition, I think a NEWS entry should be added so that people know about the fact that an AttributeError won't be raised (but for now I'll just add the skip news label)

@JelleZijlstra
Copy link
Member

Yes, this should have a NEWS entry (and therefore, an issue) as it changes user-visible behavior.

@picnixz picnixz removed the skip news label Sep 26, 2024
@hugovk hugovk changed the title tomllib.loads: Raise TypeError not AttributeError. Improve message gh-124835: tomllib.loads: Raise TypeError not AttributeError. Improve message Oct 1, 2024
@hukkin
Copy link
Contributor Author

hukkin commented Oct 1, 2024

Thanks! I've opened an issue (#124835) and added a NEWS entry.

Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@hauntsaninja hauntsaninja merged commit 9ce9020 into python:main Oct 2, 2024
36 checks passed
@hukkin hukkin deleted the tomllib-typeerror branch October 2, 2024 03:49
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.

4 participants