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

[Python] Restore PythonServerOperationErrorGenerator extending from ServerOperationErrorGenerator #2491

Closed
pose opened this issue Mar 23, 2023 · 5 comments
Assignees
Labels
python-server Python server SDK

Comments

@pose
Copy link
Contributor

pose commented Mar 23, 2023

As part of the #2336 refactor, PythonServerOperationErrorGenerator no longer extends/inherits from ServerOperationErrorGenerator:

class PythonServerOperationErrorGenerator(
    private val model: Model,
    private val symbolProvider: RustSymbolProvider,
    private val operation: OperationShape,
- ) : ServerOperationErrorGenerator(model, symbolProvider, symbolProvider.toSymbol(operation), listOf()) {
- 
+ ) {
    private val operationIndex = OperationIndex.of(model)
    private val errors = operationIndex.getErrors(operation)

Each time we change ServerOperationErrorGenerator we now need to remember to mirror the same changes on PythonServerOperationErrorGenerator which is a rather fragile process and quite likely to be missed.

Unfortunately, we cannot simply extend from ServerOperationErrorGenerator as its render method is not open. How can we make PythonServerOperationErrorGenerator extend from ServerOperationErrorGenerator on a way we could override the required behavior for the Python and TypeScript cases?

@crisidev
Copy link
Contributor

I'll assign this to me as I can bundle this up with a couple of bugfixing I am working on.

@crisidev crisidev added the python-server Python server SDK label Mar 23, 2023
@crisidev crisidev self-assigned this Mar 23, 2023
@crisidev crisidev changed the title Restore PythonServerOperationErrorGenerator extending from ServerOperationErrorGenerator [Python] Restore PythonServerOperationErrorGenerator extending from ServerOperationErrorGenerator Mar 23, 2023
@jdisanti
Copy link
Collaborator

I think we should make ServerOperationErrorGenerator configurable like we do with the things in codegen-core, and break away from the inheritance to override mentality.

@crisidev
Copy link
Contributor

I am of the opposite opinion here. We want to be able to piggy back on as much code as we can from the Rust server implementation for Python and other languages and inheritance seems to me to be filling this gap perfectly.

@hlbarber
Copy link
Contributor

hlbarber commented Mar 24, 2023

One thing to consider here is that we don't always need to extend the Rust generators to TypeScript/Python generators if we're leaving the Rust code intact and just adding code. Instead we can extend the shape visitor so that we call extra, completely separate, generators.

Examples: here and here

@crisidev
Copy link
Contributor

I think this can be closed, I refactored the code and the inheritance is not really needed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python-server Python server SDK
Projects
None yet
Development

No branches or pull requests

4 participants