Hey folks,
Is it intended that WarningMessage within warnings isn’t documented? Its also the type yielded within the list for warnings.catch_warnings. I noticed the object was a warnings.WarningMessage, but couldn’t find any docs on that type.
That code is so old I wouldn’t be surprised if no one knows.
Indeed, it’s ancient. It used to be part of Lib/test/support.py, so I presume it was for internal consumption only, and it’s accidental that it is exposed at all (it’s not in __all__). I think @vstinner last changed it – there are some mentions in the 3.6 what’s new from him.
That makes sense as long as it isn’t exposed. Since its exposed via catch_warnings I figure maybe now we should document/expose it.
I dunno. Maybe it’s supposed to be an opaque detail?
The docs give the details of what interface the returned values will provide:
Each object in the list has attributes with the same names as the arguments to
showwarning().
This seems like a classic example of a duck-typed return value, you know what you can do with it but how it’s implemented is considered an internal detail.
I don’t see why it’s important that the type gets documented beyond this - unless it’s for reasons to do with typing, which tends to demand explicit types? In which case, maybe a Protocol could be used rather than documenting the type itself? That may well be over-complicating things, though.
Yeah, I have a function that wraps catch_warnings and I tried to figure out what the type was that it was giving back and found MessageWarning. Though it didn’t seem right to use that in my type hint since it wasn’t documented.
I guess I figured it should be an exported/documented piece if it was given back by a published api.
Edit: I guess a protocol could be used, but why not instead just add MesageWarning to the api? If it changes (or the return value for catch_warnings changes) it needs to be documented anyways.
Yeah, ISTM if the function is a public API we should document it properly, unless we have a compelling reason to not document the return type or declare the type an implementation detail. Assuming we don’t have such a reason, documentation the class directly seems the simplest and most straightforward approach unless we have particular reason to believe it should instead be a protocol and the class is likely to get swapped out. And if there is a reason to undocument it, we should be explicit about that in authoritative reference documentation, rather than leaving it as an unknown gray area.
FWIW, from a type checking perspective the class is currently exposed and used as-is rather than as a protocol (see typeshed), but first and foremost it seems more straightforward to explain and document as such in user-facing docs than declaring it a protocol, unless we actually do have a reason for it (which at least so far it appears we don’t).
There were initially two custom objects: WarningsRecorder and WarningMessage. The former was replaced by a pure list and the reference to the latter was removed from docstrings in 1cd0247a4d1b8282631707ba06b514aeddc75782 (bpo-3781). The reason was: “This makes the API simpler to use as no special object must be learned.”
Ah, thanks for digging that up! For reference, the full original commit message (by @brettcannon himself, no less!) was:
warnings.catch_warnings() now returns a list or None instead of the custom
WarningsRecorder object. This makes the API simpler to use as no special object
must be learned.
The latter rationale was in reference to the replacement of the special WarningsRecorder object with a regular Python list, rather than anything directly to do with the removal of the reference to WarningMessage (which apparently hadn’t been documented by then, either). The BPO discussion only references WarningMessage once, in a discussion of API behavior, and the one docs-relevant change in the commit was replacing a reference to the undocumented class with a brief informal description of what the class contains (which had been previously documented in the now-removed WarningsRecorder class).
Here’s the relevant diff snippit (re-ordered for ease of interpretation), for reference:
- Context manager returns an instance of warnings.WarningRecorder which is a
- list of WarningMessage instances. Attributes on WarningRecorder are
- redirected to the last created WarningMessage instance.
+ The objects appended to the list are arguments whose attributes
+ mirror the arguments to showwarning().
The change makes sense at the time, as it rescues the brief informal summary of the class’ attributes from the removed WarningsRecorder class to this parameter description, where it is referenced, and is better than leaving readers completely in the dark. On the other hand, it doesn’t seem to present any compelling reason why WarningsRecorder should be kept undocumented indefinitely, given it is part of a public API, would have avoided confusion for users like @csm10495 and presuming we have someone willing to do so.
Cool. Sounds like no objections to documenting. When I get around to it, I’ll file a PR.
If we make concrete MessageWarning the documented API, then if that type ever changes, the docs need to change (which makes it a breaking change with deprecation period).
If we make the protocol the documented API, then the type can change without breaking anything, provided the protocol doesn’t change.
My preference is for the protocol (my underlying assumption is that change is inevitable and should be managed, and this is a good way to manage it), but I also don’t feel strongly about this particular case.
I’m not really sure the difference at that point. If we had a protocol and a breaking change was made, we would need a deprecation period. Seems the same for a concrete object here.
If we needed to have internal implementation details that change in a backwards compatible way, they could still exist and be underscored away.
Any non backwards compatible change to a protocol or concrete class would need a deprecation period.
If we say the return type is MessageWarning and we decide we later want to return (a hypothetical) LazyMessageWarning in some cases, that’s a breaking change.
If we say the return type has a certain set of attributes, and we later decide to return LazyMessageWarning in some cases, that is not a breaking change.
The guarantees are entirely in the contract of the API, and not the implementation of the API. This is what makes Python so much more powerful/expressive than other languages.
(Adding to the set of attributes is also not a breaking change. However, removing from the set of attributes or changing the semantics of any that are currently specified would be breaking changes in both cases.)
I see. Fair enough. If we want a protocol it’s more/less the same, just add another class inheriting from protocol instead with those attributes and document it.
Do all protocols live in typing or can they live near their use (in warnings here)?
Would it be called WarningMessageProtocol? (The name WarningMessage is nice but taken.)
It can live in typeshed, as far as I’m concerned. Until we’re embedding type hints directly in the stdlib, there’s no particular reason to put a protocol type directly in the stdlib either. It doesn’t get used or need to exist at runtime.
The typing SIG is probably the place to ask to see if they have ideas about where to put typing types in the stdlib. There might be a convention around submodules already (as we kind of have for ABCs).
The point of this thread was to document properly document the return object for catch_warnings. Having a reference to it in typeshed isn’t as useful in the context I’m asking about.
Consider the change that I have in gh-117146: Expose/document warnings.WarningMessage by csm10495 · Pull Request #117147 · python/cpython · GitHub.
Would a happy medium be to just change the phrasing from ‘WarningMessage object’ to ‘following the WarningMessage protocol’? (or similar)
Then instead of defining WarningMessage the class, define it in the docs as a Protocol?
I don’t see what’s insufficient about “Each object in the list has attributes with the same names as the arguments to showwarning().”
Formalising that only makes it harder for the reader to understand what attributes are actually available, because it adds another conceptual step between the function that creates the warning and the function that captures the warning. Right now, the relationship is incredibly direct, but adding a concrete type in between disconnects them - very important to do for extensible APIs, but not at all important for APIs like this that are only internally extensible.
So to go back to the original question of this thread, I’d simply say that it’s intentional that WarningMessage is not documented, and there’s no reason to document it.