Skip to content

GH-8626: Provide cleaner transform() DSL #8653

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 26, 2023
Merged

GH-8626: Provide cleaner transform() DSL #8653

merged 3 commits into from
Jun 26, 2023

Conversation

artembilan
Copy link
Member

Fixes #8626

  • Add missed transform(String beanName, @Nullable String methodName) API
  • Introduce a TransformerSpec to expose a strict API to configure transformer variants.
  • Introduce transformWith(Consumer<TransformerSpec>) as a single point of all possible transformer and its endpoint options
  • Deprecate those IntegrationFlowDefinition.transform() variants which are harder to configure as several lambda arguments

This change will make Kotlin & Groovy DSL more readable and straightforward

@artembilan artembilan added this to the 6.2.0-M1 milestone Jun 16, 2023
@artembilan
Copy link
Member Author

@garyrussell ,

I have a naming problem 😄

my new DSL method is transformWith(Consumer<TransformerSpec> transformerConfigurer).
If I leave it just as transform(), the "typeless" lambda style configuration would clash with an existing transform(GenericTransformer<S, T> genericTransformer).
Therefore only way to satisfy compiler is to give the method a different name.

So, is that transformWith OK? Or do you see any other possible names?

NOTE: The change in not done yet.

  1. Revise tests in other modules.
  2. Fix docs
  3. The Kotlin & Groovy DSLs might be done in the separate PR.

I don't deprecate many other transform() methods because I call them shortcuts. It is really more straightforward just to type .<String, String>transform(String::toUpperCase), then try something like .transformWith(t -> t.<String, String>transformer(String::toUpperCase)).
Or similar for just expression, or bean method call.
Probably for Kotlin & Groovy we won't have them since it looks like their DSL is more about options and their values rather then method calls.

Thanks

@artembilan artembilan marked this pull request as draft June 16, 2023 17:21
@garyrussell
Copy link
Contributor

transformWith() is fine for me. Should I wait for other changes before reviewing?

@artembilan
Copy link
Member Author

Yes, please.
I've already applied the change for Groovy DSL to justify it.
So, only Kotlin is left and docs.

Thanks

@artembilan
spring-projectsGH-8626: Provide cleaner transform() DSL
19e350c 
Fixes spring-projects#8626

* Add missed `transform(String beanName, @nullable String methodName)` API
* Introduce a `TransformerSpec` to expose a strict API to configure
transformer variants.
* Introduce `transformWith(Consumer<TransformerSpec>)` as a single point
of all possible transformer and its endpoint options
* Deprecate those `IntegrationFlowDefinition.transform()` variants
which are harder to configure as several lambda arguments

This change will make Kotlin & Groovy DSL more readable and straightforward
@artembilan
* Use new transformWith() in tests where a deprecated API still used
62c21ae 
* Add JavaDocs to `TransformerSpec`
* Fix generic types for `BaseIntegrationFlowDefinition.transformWith()` -
they make sense exactly on the `TransformerSpec.transformer()` only
* Apply `transformWith()` for Groovy DSL
* Introduce a new `ClassUtils.isLambda(Object candidate)`
and add a check for Groovy `Closure`
* Fix `GroovyIntegrationFlowDefinition.createConfigurerIfAny()` to propagate a `Consumer` argument
down to the `Closure`
@artembilan
* Rename TransformerSpec -> TransformerEndpointSpec for better context
aa219bf 
meaning of the class
* Introduce `KotlinTransformerEndpointSpec` as an extension of the `TransformerEndpointSpec`
to have an `inline fun <reified P> transformer(crossinline function: (P) -> Any)`
for Kotlin style
* Add `KotlinIntegrationFlowDefinition.transformWith(KotlinTransformerEndpointSpec)`
* Deprecate Kotlin methods which are covered by the mentioned `transformWith()`
* Fix tests to use new API
* Mentioned the change in the doc
@artembilan artembilan requested a review from garyrussell June 23, 2023 18:56
garyrussell
garyrussell approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; needs to be removed from Draft mode for merge.

@garyrussell garyrussell marked this pull request as ready for review June 26, 2023 15:49
@garyrussell
Copy link
Contributor

Removed from draft mode - let me know if that is not what you intended.

@artembilan
Copy link
Member Author

Removed from draft mode

Yes. That is exactly what we need now.
Feel free to merge.
Thanks.

@garyrussell garyrussell merged commit 070c1c6 into spring-projects:main Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyrussell garyrussell garyrussell approved these changes

Assignees
No one assigned
Labels
in: dsl type: enhancement
Projects
None yet
Milestone
6.2.0-M1
Development

Successfully merging this pull request may close these issues.

Add DSL transform() variant overload for accepting a bean name
2 participants
@artembilan @garyrussell