Refactor Instantiator
#1863
Refactor Instantiator
#1863
Conversation
The `Instantiator` is a rather old part of the codebase that has been accumulating a bit of technical debt. This commit refactors `Instantiator.kt`. * Removes dependency on `CodegenTarget`; the instantiator is now client and server agnostic. `ClientInstantiator` and `ServerInstantiator` instantiate the underlying instantiator with the settings needed by each project. * The test suite has been completely rewritten to make use of the newer testing infrastructure (`TestWorkspace.testProject()`), and coverage around enum generation has been improved. * `Instantiator.Ctx` and its uses have been simplified. * The modified code has been reformatted to adhere to our styling conventions. This commit also renames `StructureGenerator.fallibleBuilder` to `StructureGenerator.hasFallibleBuilder`. All in all this commit will make the diff of #1342 lighter, where deviations among clients and servers regarding structure instantiation substantially increase.
| rust("#T::from($data)", enumSymbol) | ||
| } | ||
|
|
||
| class ClientInstantiator(val codegenContext: CodegenContext) : |
There was a problem hiding this comment.
I'm using inheritance for ClientInstantiator and ServerInstantiator just as convenient sugar to instantiate Instantiator with the settings that each project should use, but this could very well be just a simple function returning an Instantiator, or use Kotlin's by-clause to implement delegation - happy to change.
This is a good opportunity to have the inheritance vs composition vs delegation discussion more broadly: there are many places in the codebase where we choose and mix semi-randomly, and it'd be nice to have some guidelines / conventions.
There was a problem hiding this comment.
Given that this isn't actually overriding any functions, I'd prefer to use composition with Kotlin's by delegation. I think we should prefer composition over inheritance in general, except for where it is significantly harder to reason about.
There was a problem hiding this comment.
Oh, Kotlin's by-clause delegation only works with interfaces :( https://stackoverflow.com/questions/46387525/why-only-interfaces-can-be-delegated-to-in-kotlin
There was a problem hiding this comment.
That's fine to me for this use-case.
There was a problem hiding this comment.
I'd personally prefer just simple functions that return Instantiators as you mentioned.
Inheritance is a much bigger hammer that we don't need here.
There was a problem hiding this comment.
Switched to regular functions: ea88e06
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
| * A function that given a symbol for an enum shape and a string, returns a writable to instantiate the enum with | ||
| * the string value. | ||
| **/ | ||
| private val enumFromStringFn: (Symbol, String) -> Writable, |
There was a problem hiding this comment.
It's not immediately clear to me why this would be configurable (i.e, different in the client vs the server).
Maybe you could add a comment about that either here or on ClientInstatiator::enumFromStringFn/ServerInstatntiator::enumFromStringFn?
| rust("#T::from($data)", enumSymbol) | ||
| } | ||
|
|
||
| class ClientInstantiator(val codegenContext: CodegenContext) : |
There was a problem hiding this comment.
I'd personally prefer just simple functions that return Instantiators as you mentioned.
Inheritance is a much bigger hammer that we don't need here.
| @@ -99,9 +98,7 @@ class ServerProtocolTestGenerator( | |||
| inputT to outputT | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
There's a few more calls to RustWriter::write instead of RustWriter::rust in this file (codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt). Do we want to change those?
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
The
Instantiatoris a rather old part of the codebase that has beenaccumulating a bit of technical debt.
This commit refactors
Instantiator.kt.CodegenTarget; the instantiator is now clientand server agnostic.
ClientInstantiatorandServerInstantiatorinstantiate the underlying instantiator with the settings needed by
each project.
testing infrastructure (
TestWorkspace.testProject()), and coveragearound enum generation has been improved.
Instantiator.Ctxand its uses have been simplified.conventions.
This commit also renames
StructureGenerator.fallibleBuildertoStructureGenerator.hasFallibleBuilder.All in all this commit will make the diff of #1342 lighter, where
deviations among clients and servers regarding structure instantiation
substantially increase.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.