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

Degeneralize server and client generators #1785

Merged
merged 6 commits into from
Oct 10, 2023

Conversation

zeal18
Copy link
Contributor

@zeal18 zeal18 commented Jul 30, 2023

The change degeneralizes the common logic for server/client generation making it possible to have different solutions for different frameworks and simplifying customizations

@github-actions github-actions bot added core Pertains to guardrail-core java-async-http Pertains to guardrail-java-async-http java-dropwizard Pertains to guardrail-java-dropwizard java-spring-mvc Pertains to guardrail-java-spring-mvc scala-akka-http Pertains to guardrail-scala-akka-http scala-dropwizard Pertains to guardrail-scala-dropwizard scala-http4s Pertains to guardrail-scala-http4s labels Jul 30, 2023
@blast-hardcheese blast-hardcheese added the major Bump major version label Aug 6, 2023
@blast-hardcheese
Copy link
Member

Pardon the delay, I'm road tripping through Italy for the next two weeks.

This will require review when I get back to a laptop, but I think you're right to do this on first glance.

Thank you for your effort here. I think the combinations of Config and which modules are enabled should be sufficient to maintain flexibility without the generalized methods, and this would give greater stability to the ABI.

The biggest downside is the inability to override a single method, which was the intent of the copy function on generators. That being said, any nontrivial use of that feature creates extraordinarily tight coupling, so maybe it's good to go.

Perhaps AST transformers in critical locations during generation would preserve customization somehow, but we can cross that bridge when we get there.

@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Attention: 345 lines in your changes are missing coverage. Please review.

Comparison is base (5209d35) 84.46% compared to head (e69b7c9) 82.41%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1785      +/-   ##
==========================================
- Coverage   84.46%   82.41%   -2.06%     
==========================================
  Files          95       93       -2     
  Lines        5885     7370    +1485     
  Branches      737      859     +122     
==========================================
+ Hits         4971     6074    +1103     
- Misses        914     1296     +382     
Files Coverage Δ
...les/core/src/main/scala/dev/guardrail/Common.scala 100.00% <100.00%> (ø)
...re/src/main/scala/dev/guardrail/core/Tracker.scala 91.30% <ø> (ø)
...a/dev/guardrail/generators/ProtocolGenerator.scala 100.00% <ø> (+1.32%) ⬆️
...yncHttpClient/AsyncHttpClientClientGenerator.scala 94.23% <100.00%> (+0.23%) ⬆️
...rs/java/dropwizard/DropwizardServerGenerator.scala 95.66% <100.00%> (+0.90%) ⬆️
...tors/java/springMvc/SpringMvcServerGenerator.scala 92.52% <100.00%> (+2.84%) ⬆️
...ators/scala/akkaHttp/AkkaHttpClientGenerator.scala 95.69% <100.00%> (+0.73%) ⬆️
...ators/scala/akkaHttp/AkkaHttpServerGenerator.scala 93.58% <100.00%> (+1.04%) ⬆️
...s/scala/dropwizard/DropwizardClientGenerator.scala 80.00% <100.00%> (+46.66%) ⬆️
...s/scala/dropwizard/DropwizardServerGenerator.scala 88.41% <100.00%> (+4.79%) ⬆️
... and 7 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blast-hardcheese
Copy link
Member

One of the features that I have struggled to maintain is the ability for the user to override a single method by using .copy(...) on a module and packaging that as their own module.

This could still be maintained by making all methods protected instead of private, as was recently proposed to resolve a usability issue in the http4s server generator, permitting forks of modules to override methods individually.

As much as I wish it were not necessary, I'm still sympathetic to this change, so with that change I think this can be merged. Hopefully it will increase maintainability enough that the reduction of symmetry between all generators will be worth it.

@blast-hardcheese
Copy link
Member

Release notes and migration guide to follow.

@blast-hardcheese blast-hardcheese merged commit 34957b4 into guardrail-dev:master Oct 10, 2023
42 of 43 checks passed
@zeal18 zeal18 deleted the more-independency branch November 26, 2023 11:07
@blast-hardcheese blast-hardcheese mentioned this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore core Pertains to guardrail-core java-async-http Pertains to guardrail-java-async-http java-dropwizard Pertains to guardrail-java-dropwizard java-spring-mvc Pertains to guardrail-java-spring-mvc major Bump major version scala-akka-http Pertains to guardrail-scala-akka-http scala-dropwizard Pertains to guardrail-scala-dropwizard scala-http4s Pertains to guardrail-scala-http4s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants