-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Implement the clear and task APIs #192
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
Conversation
c8d257c to
e0227ff
Compare
|
||||||||||||||
0cf4071 to
fe6390a
Compare
|
@ThomasVitale / @oscerd one thing I went back and forth on was the best way to model things that are path/query parameters. Most of the other operations are With the task & clear apis, some of the uris have path & query parameters, and mostly are @lombok.Builder(toBuilder = true)
@lombok.Getter
@lombok.ToString
public class TaskStatusPollRequest {
public static final Duration DEFAULT_STATUS_POLL_WAIT_TIME = Duration.ZERO;
@lombok.NonNull
private String taskId;
@lombok.Builder.Default
@lombok.NonNull
private Duration waitTime = DEFAULT_STATUS_POLL_WAIT_TIME;
}And the corresponding operation is defined as In this case there isn't any serialization because the object itself is never serialized. The Would it be better to not include things that are path/query params in these request objects, and instead make them parameters of the operation itself? So TaskStatusPollResponse pollTaskStatus(TaskStatusPollRequest request);would become TaskStatusPollResponse pollTaskStatus(String taskId, Duration waitTime);It makes the method more fragile, but that might be a good thing since these are required things in the API. The more I think about it the more I think we should only model things that are part of a request/response body. These other things should go directly into the operation's method signature. Thoughts? |
Also enabled parallel builds for performance Fixes docling-project#188 Signed-off-by: Eric Deandrea <eric.deandrea@ibm.com>
Also enabled parallel builds for performance Fixes docling-project#188 Signed-off-by: Eric Deandrea <eric.deandrea@ibm.com>
68defba to
2e6f562
Compare
Also enabled parallel builds for performance Fixes docling-project#188 Signed-off-by: Eric Deandrea <eric.deandrea@ibm.com>
|
After thinking about it more, I decided to go ahead and make the change. Things that are required that are outside the request body (i.e. path/query params) I added directly to the method signature. |
Also enabled parallel builds for performance Fixes docling-project#188 Signed-off-by: Eric Deandrea <eric.deandrea@ibm.com>
|
I started to work on the async support part, I'll have to aligned with this once it is merged. |
Signed-off-by: Eric Deandrea <eric.deandrea@ibm.com>
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
:java_duke: JaCoCo coverage report
|
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
|
🎉 This issue has been resolved in |
Also enabled parallel builds & build caching for performance.
As part of this I did some refactoring as well (all backwards-compatible).
DoclingServeApiinto smaller interfaces that match the operations within docling serve (chunk, clear, convert, health, task)DoclingServeApisimply extends all of them, but the operation definitions have been moved down into separate interfaces.docling-serve-apito implement the parts of the api that they want to.I did a similar refactoring to match in
DoclingServeClient.DoclingServeClientstill implements all of the methods directly, but it delegates to variousOperationsclasses that implement the actual apis.All-in-all it doesn't affect functionality, but I feel it improves readability & maintainability.
I also refactored
AbstractDoclingServeClientTeststo introduce@Nestedclasses, again 1 each for the various APIs.Fixes #188