Conversation
| #[new] | ||
| fn __new__(translation_backend: Option<PyTranslationBackend>) -> PyResult<Self> { | ||
| Ok(Self(TranslationOptions { | ||
| translation_backend: translation_backend.map(|b| b.0), |
There was a problem hiding this comment.
Typically, we use the PyWrapper as_inner() method to get at the inner value of a wrapped type:
| translation_backend: translation_backend.map(|b| b.0), | |
| translation_backend: translation_backend.map(|b| b.as_inner()), |
| @@ -0,0 +1,45 @@ | |||
| from typing import Optional, final | |||
|
|
|||
| @@ -0,0 +1,45 @@ | |||
| from typing import Optional, final | |||
There was a problem hiding this comment.
This file is orphaned. The grpc and models directory both need __init__.pyi files exporting models, and translation, respectively.
| def from_v1(inner: BackendV1Options) -> "TranslationBackend": ... | ||
| @staticmethod | ||
| def from_v2(inner: BackendV2Options) -> "TranslationBackend": ... | ||
|
|
| """ | ||
| Options for the V1 translation backend. | ||
| """ | ||
|
|
| - ``from_*``: wrap underlying values as this enum type. | ||
|
|
||
| """ | ||
|
|
There was a problem hiding this comment.
| def inner(self) -> Union[BackendV1Options, BackendV2Options]: ... |
| submodules: [ | ||
| "client": client::init_submodule, | ||
| "compiler": compiler::init_submodule, | ||
| "grpc": grpc::init_submodule, |
There was a problem hiding this comment.
I know you are just carrying forward with an existing module and just actually exporting it here, and I know the plan is to add a wrapped type later, but I don't love exporting raw bindings to the API here, and especially exposing them as a "GRPC" module to users.
Other modules get coupled to it because by it's very nature it contains models that requests across the stack need, but the individual models are typically scoped to a single kind of request, so why have them all co-located? It's also ambiguous when you need to pull it in, since our services use a mix of REST and GRPC.
To me, it feels like we're exposing unnecessary implementation details to users and making it more complex than needed to reason about our API and how to use it. translation is a clean concept within the scope of our product but grpc is ambiguous and doesn't help users discover useful things about our API.
I could be convinced to re-export the needed GRPC model directly from the translation module, that way it is at least grouped within a concept, reducing the number of imports needed and making it easier to find the needed type with tooling.
There was a problem hiding this comment.
Agreed. Went in that direction on a new PR, #308
3 participants
Closes #306
Closes #281
translategrpcmodule into Python bindingsTODO:
grpc/controller. Will add that once I know these changes are on the right track