gh-124835: tomllib.loads: Raise TypeError not AttributeError. Improve message#124587
gh-124835: tomllib.loads: Raise TypeError not AttributeError. Improve message#124587hauntsaninja merged 4 commits intopython:mainfrom
tomllib.loads: Raise TypeError not AttributeError. Improve message#124587Conversation
picnixz
left a comment
There was a problem hiding this comment.
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
loadsshould 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.
|
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) |
|
Yes, this should have a NEWS entry (and therefore, an issue) as it changes user-visible behavior. |
tomllib.loads: Raise TypeError not AttributeError. Improve messagetomllib.loads: Raise TypeError not AttributeError. Improve message
|
Thanks! I've opened an issue (#124835) and added a NEWS entry. |
This PR makes
tomllib.loads(b"")throwinstead of
Given an input type that doesn't have the
replaceattribute, it makestomllib.loadsraise 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)
tomllib.loads#124835