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

Yaml support #10

Merged
merged 6 commits into from
Jan 4, 2017
Merged

Yaml support #10

merged 6 commits into from
Jan 4, 2017

Conversation

sypticus
Copy link
Collaborator

@sypticus sypticus commented Jan 4, 2017

Now allowing httpRules to be defined in a YAML file instead of the .proto files.

Copy link
Owner

@Xorlev Xorlev left a comment

Choose a reason for hiding this comment

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

Overall I'd like if you could take a pass for style consistency, but otherwise this is looking pretty good.

import java.util.Set;

/**
* Created by kylehansen @Sypticus on 12/28/16.
Copy link
Owner

Choose a reason for hiding this comment

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

Lets make this javadoc relevant -- a good candidate would be at least a link to the doc page mentioning the YAML format.

/**
* Created by kylehansen @Sypticus on 12/28/16.
*/
@Data
Copy link
Owner

Choose a reason for hiding this comment

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

This could be @Value instead.

import java.util.stream.Collectors;
/**
* Created by kylehansen @Sypticus on 12/28/16.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto on Javadoc, just a little bit about why it exists.

//Defined in Yaml
}
rpc TestMethod5 (TestRequest) returns (TestResponse) {
//Defined in Yaml
Copy link
Owner

Choose a reason for hiding this comment

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

Comments are inconsistent spacing.

if(rule.getSelector().equals(fullMethodName) || rule.getSelector().equals("*")){ //TODO: com.foo.*
//YAML http rules override proto files. - https://cloud.google.com/endpoints/docs/grpc-service-config
DescriptorProtos.MethodOptions yamlOptions = DescriptorProtos.MethodOptions.newBuilder().setExtension(AnnotationsProto.http, rule.buildHttpRule()).build();
methodProto = DescriptorProtos.MethodDescriptorProto.newBuilder().mergeFrom(methodProto).setOptions(yamlOptions).build();
Copy link
Owner

Choose a reason for hiding this comment

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

This lines are really really long, I'd like to keep it at ~120 chars.

@@ -85,6 +83,16 @@
for(Descriptors.ServiceDescriptor serviceDescriptor : fd.getServices()) {
DescriptorProtos.ServiceDescriptorProto serviceDescriptorProto = serviceDescriptor.toProto();
for(DescriptorProtos.MethodDescriptorProto methodProto : serviceDescriptorProto.getMethodList()) {
String fullMethodName = serviceDescriptor.getFullName() +"." + methodProto.getName();
if(yamlConfig.isPresent()){ //Check to see if the rules are defined in the YAML
for(YamlHttpRule rule :yamlConfig.get().getRules()){
Copy link
Owner

Choose a reason for hiding this comment

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

Space after : -- I also noticed that most lines are lacking a space between ) and {. I don't have a problem with it in spirit, but I would like to keep the style consistent.

@sypticus
Copy link
Collaborator Author

sypticus commented Jan 4, 2017

Done

@Xorlev
Copy link
Owner

Xorlev commented Jan 4, 2017

One last comment remaining on a Javadoc

@sypticus
Copy link
Collaborator Author

sypticus commented Jan 4, 2017

Changes were below you marker. But cleaned them up further.

@Xorlev Xorlev merged commit a030918 into master Jan 4, 2017
@Xorlev Xorlev deleted the yamlSupport branch January 4, 2017 23:30
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.

2 participants