-
Notifications
You must be signed in to change notification settings - Fork 218
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
Feature request: includeFor integration #2318
Comments
The problem is that there's not really a good generic place to run this sort of thing. If you were to run it before You could use the configure hook to bail early on settings, such as a configured service. |
There's not really a super clean and robust way of offering this generically. The most flexible way would be to add something like this to the interface: default boolean enabled() {
return true;
} Then we could skip over any that return false, and you'd be able to change that whenever you like based on whatever reason. Alternatively (or additionally) we could provide some base classes that have methods for you to implement on configuration or when the model's ready. They'd be some really odd interfaces though. A wrapping class might be better. |
I was suggesting that all integrations run includeFor, and then whatever ones were selected, then run preprocessModel on all of those? (not includeFor then preprocess for each integration). Would that work?
It would be nice to inspect the model and/or the service shape, so we can check "Does this apply to S3?" or some other service. Our interface is a boolean. |
There is still the problem of preprocess changing the model such that a given integration is now valid or is now no longer valid. For example, imagine you have an integration that generates a protocol implementation for So you can't provide the model before preprocess is called. You can provide the settings, which don't change and which do include the service id, but there's plenty of reason to need the final model (like requiring the presence of a protocol trait). We could provide hooks at both points in time, one with just settings and one with full context. Ideally in some way where the post-preprocess method forbids use of preprocess. The question is how much foot-gunning do we want to accept. |
I understand your concern. I don't really have a preference, but there is a valid need for integrations to only apply to a single service, as Ruby and Kotlin (and others?) have done. I still think it's reasonable to have includeFor run for all integrations and where true, only apply those to the service before any pre processing, even if pre processing would otherwise make it valid in that case. |
It is useful to have an integration only apply for a specific service.
We have this in Ruby like so: https://github.com/smithy-lang/smithy-ruby/blob/779dd349560371b525758a45e83e21673e67753a/codegen/smithy-ruby-codegen/src/main/java/software/amazon/smithy/ruby/codegen/RubyIntegration.java#L45
Kotlin also has this: https://github.com/smithy-lang/smithy-kotlin/blob/main/codegen/smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/integration/KotlinIntegration.kt#L60
I'm unsure of the recommended method arguments for this, but the need is for integrations to only apply in some cases, such as s3, or if a service has a particular trait. Such a method (I propose includeFor) should run before preprocessModel and other integration hooks - it should be the very first thing checked IMO.
Currently we run into issues using preprocessModel, which runs before our hook to includeFor, so each preprocessModel method has to check and return the model if certain shapes aren't present or if the service model isn't what the integration is targeting.
The text was updated successfully, but these errors were encountered: