Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

fixes #58: upgrade client dependency #71

Merged
merged 1 commit into from
Nov 12, 2016

Conversation

dcrissman
Copy link
Member

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.584% when pulling cef1e6a on dcrissman:clientUpgrade into 151d887 on esbtools:master.

@@ -85,7 +85,7 @@ public BulkLightblueRequester(LightblueClient lightblue) {
}

@Override
public TransformableFuture<LightblueDataResponses> request(AbstractLightblueDataRequest... requests) {
public TransformableFuture<LightblueDataResponses> request(CRUDRequest... requests) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to use the most top level, but still relevant type here, as in, what the client accepts for data(..) calls. So, I think we should be using LightblueDataRequest instead of CRUDRequest for this type (and everywhere else this occurs).

Copy link
Member Author

Choose a reason for hiding this comment

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

I am looking to see if I can use LightblueDataRequest anywhere, but CRUDRequest has the getOperation method which is required in DataBulkRequest.

Copy link
Member Author

Choose a reason for hiding this comment

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

You cannot use DataBulkRequest with anything other than a CRUDRequest, and bulk operations appear to be what the EventHandler users.

That said, the below interfaces could each be altered to take a generic that extends LightblueDataRequest. Then BulkLightblueRequester could just use CRUDRequest. We could go an extra step and create interface helpers for CRUDRequest. See example below.

Honestly, it seems a bit much to me as BulkLightblueRequester is currently the only implementation of LightblueRequester.

  • LightblueDataResponses
  • LightblueRequester
  • LightblueResponses

Example

public interface LightblueRequester<T extends LightblueDataRequest> extends Requester<T, LightblueDataResponse> { ... }

public interface LightblueCrudRequester extends LightblueRequester<CRUDRequest> {}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine with CRUDRequest, I didn't realize it was required for bulk requests. There is already a generic Requester interface in case another type is needed.

@alechenninger
Copy link
Contributor

LGTM! Thanks!

@alechenninger alechenninger merged commit 60b1e66 into esbtools:master Nov 12, 2016
@dcrissman dcrissman deleted the clientUpgrade branch November 12, 2016 13:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants