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

Add example #35

Open
wants to merge 6 commits into
base: add-example
Choose a base branch
from
Open

Conversation

eoincav
Copy link

@eoincav eoincav commented Jun 14, 2020

Initial merge of a simple arithmetic server.

@lucasdicioccio
Copy link
Member

Congratz on completing your first end-to-end test =).

Would you mind adding the .proto interface?

@eoincav
Copy link
Author

eoincav commented Jun 14, 2020 via email

@lucasdicioccio
Copy link
Member

lucasdicioccio commented Jun 15, 2020

No need for a new-pull request as long as you push to the same remote/branch !

Thanks for adding the protos. I'll soon merge this branch after another review. Then, I'll do one pass on the overall structure before merging the add-example branch in the main one.

@lucasdicioccio
Copy link
Member

I've added some comments, if you feel it's too much work or if there are things unclear ping me and we can get through the comments together.

I'm thinking about one improvement (I don't mind pairing with you for adding this) is to have one (or two) executables for running the examples. Also, once we have the executables we should build these inside the CI jobs but then this is again another story.

@eoincav
Copy link
Author

eoincav commented Jun 16, 2020

I've added some comments, if you feel it's too much work or if there are things unclear ping me and we can get through the comments together.

I'm thinking about one improvement (I don't mind pairing with you for adding this) is to have one (or two) executables for running the examples. Also, once we have the executables we should build these inside the CI jobs but then this is again another story.

HI .. I was planning to add binary executables anyway, so fine with that. I should be good to do that on my own. Can't see specific comments in the PR code. Am I being dense?

}
```

Each RPC call takes a single message, and returns a single message. In this case the request is a *CalcNumbers* and the response is a *CalcNumber*. Protobuf allows message types to be nested. In this case *CalcHumbers* is comprised of a list of *CalcNumber* values. *CalcNumber* itself is a [32 bit unsigned int](https://developers.google.com/protocol-buffers/docs/proto3#scalar_value_types).
Copy link
Member

Choose a reason for hiding this comment

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

s/CalcHumbers/CalcNumbers

Each RPC call takes a single message, and returns a single message. In this case the request is a *CalcNumbers* and the response is a *CalcNumber*. Protobuf allows message types to be nested. In this case *CalcHumbers* is comprised of a list of *CalcNumber* values. *CalcNumber* itself is a [32 bit unsigned int](https://developers.google.com/protocol-buffers/docs/proto3#scalar_value_types).

# Getting it working
The *calcs.proto* definition file is used to generate Haskell code via the *protoc* tool, which needs to be installed separately. There are two Haskell files generated. The simplest way to build the client and server is to copy these generaeted source files to somewhere accessible within your stack package.
Copy link
Member

Choose a reason for hiding this comment

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

I think what you should say in this paragraph at this point is:

  • this repository contains the generated Haskell code. Thus, the example requires no specific tooling not code-generation steps.
  • however we encourage users to try re-generating the code to practice this step
  • the code-generation requires to install protoc and proto-lens-protoc separately. Both binaries must be on the PATH before invoking protoc

# Getting it working
The *calcs.proto* definition file is used to generate Haskell code via the *protoc* tool, which needs to be installed separately. There are two Haskell files generated. The simplest way to build the client and server is to copy these generaeted source files to somewhere accessible within your stack package.

## Starting the Server
Copy link
Member

Choose a reason for hiding this comment

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

here you need to say "in an interactive session" because you use ghci below

someFunc' []
~~~

This starts the server on port 3000.
Copy link
Member

Choose a reason for hiding this comment

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

Add something about the fact that the server runs waiting for client queries (and hence does not give the prompt back).

~~~
stack ghci
:l ArithServer
someFunc' []
Copy link
Member

Choose a reason for hiding this comment

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

maybe rename the function to something more "user friendly" like runExampleServer


~~~
stack ghci
ArithClient.main
Copy link
Member

Choose a reason for hiding this comment

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

same as above,

maybe rename the function to something more "user friendly" like runExampleClient

@lucasdicioccio
Copy link
Member

Sorry about the comment not show. They appeared on my view but I had to press a "submit review" button. Which is new to me (I'm more used to GitLab review system). Sorry about the delay.

@eoincav
Copy link
Author

eoincav commented Jun 19, 2020 via email

@ProofOfKeags
Copy link
Collaborator

What still needs to be done with this? I'm trying to orient myself to this library right now as I need gRPC for what I'm doing and I'm having a difficult time doing so. I imagine that this PR with an example could help people like me. It seems from the comment stream that this is mostly ready to go save for some polish. I'd love to help but I don't think I can push to the fork branch.

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