Repository navigation
Mark deprecated services and methods in generated code #382
Description
Activity
Thanks for bringing this up, I didn't realize the decorator is available from typing_extensions. It's tricky at least in protobuf-py, where normal fields are very messy to handle (constructor overloads, replacing attribute with
@property/@property.setterto be able to apply a decorator) or seemingly impossible for enum values, extensions, oneof fields.So we would need to decide if something partial is ok / how much we would want consistency
- Whether to use
@deprecatedhere even if protobuf-py doesn't - Whether to use
@deprecatedon what's easy only across both repos, types and RPC methods - Add the messiness for fields at least. Accept that coverage still isn't complete
In my experience, fields are commonly deprecated. Getting a notice in type checkers would be quite nice indeed so perhaps there is bang for buck in supporting them. I think enum values are also relatively commonly deprecated though and the inconsistency feels unfortunate. And I am not excited to only mark message / enums deprecated and not fields.
I am currently leaning towards not annotating, both here and in protobuf-py, treating them as one shared cosystem, and not wanting to annotate in a half-baked way. In which case I would suggest going with the text form. But what do you think?
For example, this is something like what handling fields would look like (with more than one deprecated field, it just gets worse)
class Msg(Message[_MsgFields]): if not TYPE_CHECKING: __slots__ = ("value", "old") if TYPE_CHECKING: @overload def __init__(self, *, value: int = 0) -> None: ... @overload @deprecated("pkg.Msg.old is deprecated.", category=None) def __init__(self, *, value: int = 0, old: int = 0) -> None: ... def __init__(self, *, value: int = 0, old: int = 0) -> None: ... value: int @property @deprecated("pkg.Msg.old is deprecated.", category=None) def old(self) -> int: ... @old.setter @deprecated("pkg.Msg.old is deprecated.", category=None) def old(self, value: int) -> None: ...
- Whether to use
yeah, I don't love the inconsistency with protobuf-py. Since most of the difficulty is upstream, it might be worth opening an issue there and noodling a little more, leaving this open in the meantime? Like you said, I don't think it's trivial (or perhaps, even possible) to support the decorator on all of the protobuf constructs supported by protobuf-py.
FWIW, it looks like mypy-protobuf only supports a subset of protobuf deprecated options as well.
Ended up warming to it, and it will be good to do it here too (probably after the next protobuf-py release for the suppression, which I think we need too for servers) - bufbuild/protobuf-py#123
protoc-gen-connectrpcignoresoption deprecated = trueon services and methods, so nothing in the generated code signals to callers that an RPC is on its way out.The other Connect implementations surface it through each language's standard marker, which editors and linters pick up:
// Deprecated: do not use.on theFooServiceClientandFooServiceHandlerinterfaces for a deprecated service, and on the interface methods for a deprecated method (main.go#L652-L753).protoc-gen-es, which adds@deprecatedto the JSDoc of a deprecated rpc, and of a service when the service or its file is deprecated (jsdoc.ts#L71-L85).[deprecated = true]to the proto provenance line in message, enum, and field docstrings, with no decorator (protoc_gen_py/__init__.py#L497-L586). It doesn't generate services, and itsDescServiceandDescMethodalready exposedeprecated.I propose applying PEP 702's
deprecateddecorator, imported fromtyping_extensions(already a runtime dependency), withcategory=Noneso there's no runtime warning, matching Go and JS. Neither of those uses the proto comment as the deprecation reason, and the comment already lands in the docstring, so the message is a fixed string naming the element:For a deprecated service, the decorator goes on the client classes and the service
Protocols (async and sync). For a deprecated method, it goes on the corresponding client andProtocolmethods. Type checkers then report constructing the client and calling deprecated methods, whether through the client or theProtocol; implementing theProtocolisn't flagged. ty reports these by default, while pyright needsreportDeprecated(on in strict mode) and mypy needsenable_error_code = ["deprecated"].The alternative is to follow protobuf-py and only note deprecation in docstrings. That keeps the two generators consistent but is invisible to type checkers, which is most of the point of the option. If the decorator approach lands here, it may be worth proposing the same for messages and fields in protobuf-py.
Open question: should
category=Nonebe the only behavior, or should a plugin option opt into a runtimeDeprecationWarning?