Closed Hakky54 closed 4 years ago
@rybalkinsd This feature was in my opinion quite challenging. It works but I don't think it is the most clever solution. What do you think?
I still need to add all the unit test, javadoc and do some additional clean-up.
Merging #189 into master will decrease coverage by
0.21%
. The diff coverage is84.61%
.
@@ Coverage Diff @@
## master #189 +/- ##
============================================
- Coverage 91.50% 91.28% -0.22%
Complexity 127 127
============================================
Files 41 44 +3
Lines 400 413 +13
Branches 49 52 +3
============================================
+ Hits 366 377 +11
Misses 9 9
- Partials 25 27 +2
Impacted Files | Coverage Δ | Complexity Δ | |
---|---|---|---|
...github/rybalkinsd/kohttp/dsl/async/HttpAsyncDsl.kt | 33.33% <33.33%> (ø) |
0.00 <0.00> (?) |
|
.../kotlin/io/github/rybalkinsd/kohttp/dsl/httpDsl.kt | 100.00% <100.00%> (ø) |
0.00 <0.00> (?) |
|
...o/github/rybalkinsd/kohttp/util/HttpContextUtil.kt | 100.00% <100.00%> (ø) |
0.00 <0.00> (?) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 8232a1a...29fe8c1. Read the comment docs.
@Hakky54
Wow! I also expected it to be more challenging.
I like the API
http(method = ... ) {
contextAction()
}
A couple of things to think about:
fun <T> http(..., method: Method<T>, init: T.() -> Unit) ...
It will probably remove a declaration dualityval r: ResultType = httpGet<ResultType> { }
@IVSivak could you please share your thoughts?
Sure, I will check if it is possible to combine the method and the context with generics.
By the way I have the feeling that HttpContextUtil is a bit over complex, it is in my opinion not very readable. What do you guys think? Do you think there is a better way with plain kotlin to have something like that working?
By the way I have the feeling that HttpContextUtil is a bit over complex, it is in my opinion not very readable. What do you guys think? Do you think there is a better way with plain kotlin to have something like that working?
It looks like a bit complicated, imo. But yeah, as I mentioned, maybe it's possible to resolve this using types.
Another option to consider is to make Method
a sort of context factory.
I refactored a bit and the resulting code is simplified compared to the initial commit. Functionality wise it is exact the same as the previous commit, the only difference is that with this commit a http method is required when calling the http function.
The flows will be:
For Get request
For Post request
For Delete request
etc...
I also wanted to check the type of init: T.() -> Unit
within the when statement, but couldn't find a good way. So what I wanted to do is for example is to create only a HttpGetContext when the method is GET and the init variable is instance of HttpGetContext.() -> Unit
or is instance of HttpContext.() -> Unit
or else throw a specific runtime exception as it does not make sense to pass a HttpPostContext when creating a GET request. Another option could be that it is still allowed to pass a HttpPostContext when creating a GET request but behind the scenes we create a new HttpContext instance from the HttpPostContext and supply it to the HttpGetContext. In that way it will be save to pass any kind of sub class of HttpContext to the http function.
What do you think of the code change and of the flows? Is it ok to throw a class cast exception when someone is being funny by passing a HttpPostContext while executing a Get request?
I like the simplified version!
I was thinking about when it's possible to use exactly same request with any request method. And it looks like strict typing is probably the thing we're looking for
fun http(client: Call.Factory = defaultHttpClient, method: Method, init: HttpContext.() -> Unit): Response {
return client.newCall(method.makeContext().apply(init).makeRequest()).execute()
}
It will only allow to use the context initialisers applicable to any context.
It's possible to make an extension function makeContext
based on createHttpContext
It could look like
internal fun Method.makeContext(): HttpContext {
return when (this) {
Method.GET -> HttpGetContext()
Method.POST -> HttpPostContext()
...
}
}
Another opportunity here is to support methods with body (inherited from HttpPostContext
)
It looks like I need to learn a lot about Kotlin, it is quite different compared to Java. I didn't know that an existing enum could be enriched with a function outside the enum declaration. Awesome!
I applied your feedback, but when I remove the generic type <T : HttpContext>
the test class fails to compile. It doesn't accept that I provide HttpPostContext to a function which accepts HttpContext.() -> Unit
In my opinion it doesn't make sense because I am sending an instance of a child class of a HttpContext and the function parameter should be saying that it should accept HttpContext and all child classes of HttpContext. Or do you think I am missing something?
I committed the changes up till now.
Your other option regarding supporting method with body by inheritance from HttpPostContext gives me the idea that it is the best option to move from here on, but looking at OOP aren't we misusing HttpPostContext? What do you think of moving the body property to HttpContext (the base class) and disable it at the child classes which don't have a body, such as the HttpGetContext.
I Added both cases mentioned within my precious comment as two separate commits:
Cases:
As a mentioned earlier, I couldn't remove the generic type within the http function for case 1 as it fail to compile wen passing a sub class of HttpContext. For example if I want to make a http post request with a body. I am forced to use the sub class HttpPostContext because the body isn't available within the base class. But without the generics I am not able to pass HttpPostContext.
I am curious what you think of the both cases.
@Hakky54
- Move body to base class and disable it for GET and HEAD requests: 92cd43a
We tried it once in a while. Users are getting frustrated :) So I would try to avoid it.
- Extending Method with a function: f383088
It looks good for me!
Just want to make sure what is the reason of T
in
internal fun <T : HttpContext> Method.createHttpContext(): T
the test class fails to compile Maybe I'm missing something, but I also expected test class not to compile and It's valid imo
http(method = Method.DELETE, init = fun HttpContext.() {
host = "postman-echo.com"
path = "/delete"
// body should not compile
body { }
}
Imagine our use case: we have an identical request and we want time to time use POST or GET to for it.
I would go with a code like
val ctx = {
host = "..."
port = "..."
param { }
}
http(method = variableMethod, ctx)
In this case the only possible ctx
type that will make sense for both cases is HttpContext.() -> Unit
Thanks again for the feedback!
Just want to make sure what is the reason of T in
internal fun
Method.createHttpContext(): T
The reason was to support child classes of HttpContext such as HttPostContext and HttpGetContext. I removed the generic type T as you already mentioned that it is fine for the initial version to only support the base class HttpContext. I dropped case number 2 and refactored it. I have tried to finalize it. Added unit tests and updated the documentation. Let me know if you are ok with the changes.
By the way I also added unit tests for all the async http requests, but somehow CodeCov isn't recognises it
By the way I also added unit tests for all the async http requests, but somehow CodeCov isn't recognises it
No worries at all, I'll check codecov later this week
I created #190 to follow import optimisations, so the impact will be wider! @Hakky54
It looks like you are enjoying all these changes 😄 Sure I will extract the additional unit test and some code cleanup to a separate branch and scope down this branch to get more focus on the actual feature.
Any idea when you will be releasing by the way?
Any idea when you will be releasing by the way?
Want to put 0.12.0
this Saturday
Hehehe, firing all the commits to make this baby ready to ship. It think I applied all your remarks. I will also push the other branches
This is a pull request to enable dynamic http request, see issue mentioned here: https://github.com/rybalkinsd/kohttp/issues/169
Summary: With the current version the end-user needs to explicitly specify which http context to use for each http method type, see here code snippet from the issue https://github.com/rybalkinsd/kohttp/issues/169:
Provided solution Within this pull request a http request can be executed by either specifying the http method or http context. The underlying function will determine during runtime which http context to use by the provided arguments.
The above snippet can be rewritten with the following snippet:
Note This pull request is currently WIP to get review input on the initial solution. Some unit tests are already added to provide example usages.