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

Implementing Consumer<Builder> client operations #280

Merged
merged 1 commit into from
Nov 13, 2017

Conversation

kiiadi
Copy link
Contributor

@kiiadi kiiadi commented Nov 10, 2017

see #278

Copy link
Contributor

@spfink spfink left a comment

Choose a reason for hiding this comment

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

Looks good other than comment on other PR around docs making it clear that .build() is being called automatically.

@kiiadi kiiadi force-pushed the kiiadi/consumer-builders-on-models branch from 0725586 to f7f0a91 Compare November 13, 2017 20:06
@kiiadi kiiadi force-pushed the kiiadi/consumer-builders-on-client branch 2 times, most recently from fa48607 to 2bfbec6 Compare November 13, 2017 20:44
.collect(Collectors.toList()));
return methods;
.collect(toList()));
return methods.stream().sorted(Comparator.comparing(m -> m.name)).collect(toList());
Copy link
Contributor

@zoewangg zoewangg Nov 13, 2017

Choose a reason for hiding this comment

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

Question: I saw some places we are doing static imports and some are not. Do we want to make it more consistent?

// TODO This is inconsistent with how async client reuses method signature
static MethodSpec.Builder operationMethodSignature(IntermediateModel model, OperationModel opModel) {
private static MethodSpec.Builder operationBaseSignature(IntermediateModel model,
OperationModel opModel,
Copy link
Contributor

Choose a reason for hiding this comment

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

Alignment of parameters is off here

Quote from the comment Sam left on my PR. 😂

@@ -37,7 +37,7 @@
*/
FILE,

/**
CONSUMER_BUILDER, /**
Copy link
Contributor

Choose a reason for hiding this comment

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

minor formatting issue

@kiiadi kiiadi force-pushed the kiiadi/consumer-builders-on-client branch from 2bfbec6 to baec9e3 Compare November 13, 2017 22:39
@kiiadi kiiadi force-pushed the kiiadi/consumer-builders-on-models branch from 73351c3 to 710b3af Compare November 13, 2017 22:42
@kiiadi kiiadi force-pushed the kiiadi/consumer-builders-on-client branch from baec9e3 to a67663e Compare November 13, 2017 22:58
@kiiadi kiiadi changed the base branch from kiiadi/consumer-builders-on-models to master November 13, 2017 22:59
@kiiadi kiiadi force-pushed the kiiadi/consumer-builders-on-client branch from a619d08 to 769658f Compare November 13, 2017 23:00
@kiiadi kiiadi merged commit d1c1124 into master Nov 13, 2017
@zoewangg zoewangg deleted the kiiadi/consumer-builders-on-client branch December 6, 2017 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants