spring-cloud-contract icon indicating copy to clipboard operation
spring-cloud-contract copied to clipboard

Some remarks regarding the Kotlin DSL

Open hgarus opened this issue 4 years ago • 6 comments

Hi,

I've recently tried to write some contracts using the Kotlin DSL and I ran into some issues, I've also thought about ways how they could be remedied.

The main reason for me to use the Kolin DSL over the Groovy DSL would be stronger typing and thus better IDE support (static type checking to catch errors before I run maven and better autocompletion for better discoverability). Being able to write the little non-DSL code - if any - I write inside contracts in Kotlin instead of Groovy would be a very minor bonus.

In my opinion the current iteration of the Kotlin DSL doesn’t provide that. Especially when it comes to discoverability it feels worse than the Groovy DSL. This may be exacerbated by the current version of IntelliJ IDEA which seems to run into performance problems when processing Kotlin-Script files.

Broadly my problems with the DSL can be separated into two categories, its use of property assignment and the fact that some of the scopes are cluttered with a lot of constant definitions.

Property Assignment

I think currently there are roughly four categories of assignable properties in the current DSL:

Properties with basic types

Properties like contract.name or response.async. These are pretty straightforward and easy to understand.

Weakly typed properties

Some properties have the type DslProperty<Any>? (request.method, response.status and response.delay). With method and status it is pretty obvious that one is supposed to assign one of the constants - more on those later. I still don't see why these aren't Enums.

Delay is worse in some ways, hidden between all of the constants is fixedMilliseconds()which as it turns out just converts a long to a DslProperty. This could just be a Long property or even better a java.time.Duration (sadly kotlin.time.Duration is still flagged as experimental). Alternatively it could be a function to allow overloading.

Properties which are (hopefully) never assigned directly

Properties like headers and cookies with no obvious way to create a value for assignment. These seem unnecessarily exposed, in theory this allows something like

headers = Headers().apply { header(“foo”, “bar”) }`

but I don’t think that is a good way to define a header.

The DSLs corresponding to these attributes feel fine and do what I’d expect them to do for the most part.

Properties which expect me to call a function with the same name

First and foremost body, there are others such as url. Personally I consider the fact that these functions don't assign the value themselves very unexpected, coming from the groovy DSL I expect

body("Some Body")

to define a body for the request/response, instead it does silently nothing.

In my opinion the currently used

body = body("Some Body")

is just more verbose for no discernable reason.

Scoping and constants

When I open a new “request block” the first thing I do is press Control+Space to find out which things I can put in there. Due to the way the DSL is currently scoped, IntelliJ happily presents all of the response code constants and all of the "value definition stuff" (v, producer, consumer, any* and so on), which are all pretty useless at the top level of a request block and drown out the stuff I actually want to see (headers, body, status etc.).

What to do?

I can think of multiple improvements to alleviate these issues, unfortunately most of those are breaking changes:

  1. Clean up the scopes: a. Remove all of the status code and method constants from request and response and move them either inside very small DSLs or replace them with enums. b. Move everything required for "value definition" to tighter scopes. I probably don't need anyInteger when I am defining the status code. This may require some small new DSLs such as body and url. A lot of the scopes which define values already have their own DSLs anyway (cookies, headers).
  2. Use attribute assignment only for basic types: If it gets assigned a String or a Boolean it's probably fine, otherwise use a function call or a DSL.

If there is interest I could get started on an implementation to make these points less abstract.

hgarus avatar Mar 05 '20 22:03 hgarus

cc @TYsewyn @sdeleuze

marcingrzejszczak avatar Mar 05 '20 22:03 marcingrzejszczak

Hi @hgarus! We'd love to see improvements in the Kotlin DSL! The current implementation of the Kotlin DSL was a first approach to have the same feature set like the Groovy contracts. The next release of the master branch will be a major one so I don't see any issues with breaking changes. I'd suggest to create a WIP pull request so we can review your changes along the way. Personally I'm not working on SC Contract on a daily basis (you might have noticed the delay in response 😅) so this would really help me out to monitor the progress you make every couple of days. 🙂

TYsewyn avatar Mar 17 '20 12:03 TYsewyn

@hgarus How's it going? Please let us know if you need any help with this issue.

TYsewyn avatar Apr 23 '20 16:04 TYsewyn

@TYsewyn I am sorry, but I have been pretty busy with other things lately. I'll try to have something for you to look at next week.

hgarus avatar Apr 27 '20 16:04 hgarus

@TYsewyn I was a bit stuck finding a place to put value related constants and functions. I've finally managed to find something I'm not totally unhappy with. I've pushed my current progress to this branch:

https://github.com/hgarus/spring-cloud-contract/tree/kotlin-dsl

I'm currently (888518075240ec36) finished with the request - at least for now.

With the value stuff out of the way I'm hopeful to finish response tomorrow. I'll probably wait for some input on those until I get started on the messaging DSLs.

hgarus avatar May 06 '20 22:05 hgarus

I just pushed a new version of ResponseDsl.

I have tried two different approaches for "Enum-like" values (Request.method and Response.status)

  1. Enums
  2. properties on a separate object each representing a constant assigning the value on a call to get()

My last commit (74f3fe01) switches to the second approach, which has the advantage of not requiring separate Imports but could be considered somewhat obscure. I am still somewhat unsure about this.

hgarus avatar May 07 '20 21:05 hgarus