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

Enhance/Improve gRPC docs containing the following changes: #4606

Merged

Conversation

klustria
Copy link
Member

  1. Add Configuration section to MP Server doc.
  2. Add more examples by providing links to various example scenarios available in the example directory of the Helidon repository.
  3. Remove any examples of Java Serialization as the custom marshaller.
  4. Remove Experimental warning from the doc.

1. Add Configuration section to MP Server doc.
2. Add more examples by providing links to various example scenarios available in the example directory of the Helidon repository.
3. Remove any examples of Java Serialization as the custom marshaller.
4. Remove Experimental warning from the doc.
Copy link
Member

@tjquinno tjquinno left a comment

Choose a reason for hiding this comment

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

A few typos that really should be fixed. Lots of (IMO) optional clean-up. (Much of what I suggest is in text you inherited as opposed to changes you've made.) For some of my notes I could not associate them directly with the relevant line (because they applied to inherited text, not your changes.) So apologies if it's hard to map from the notes to the lines. I started adding the line numbers explicitly in the note to be clearer!

docs/se/grpc/client.adoc Outdated Show resolved Hide resolved
docs/se/grpc/client.adoc Outdated Show resolved Hide resolved
docs/se/grpc/client.adoc Show resolved Hide resolved
docs/se/grpc/client.adoc Show resolved Hide resolved
docs/se/grpc/client.adoc Outdated Show resolved Hide resolved
docs/se/grpc/server.adoc Show resolved Hide resolved
docs/se/grpc/server.adoc Show resolved Hide resolved
docs/se/grpc/server.adoc Outdated Show resolved Hide resolved
docs/se/grpc/server.adoc Show resolved Hide resolved
docs/se/grpc/server.adoc Outdated Show resolved Hide resolved
1. Resolve all review feedback.
2. Provide more details on the Marshalling section.
3. More updates and corrections.
@klustria
Copy link
Member Author

Appreciate the thoroughness of your review @tjquinno. I made additional proof-reading and found some opportunities to improve further. Also, I added more details on how to set the custom marshaller on the Marshalling documentation based on whether it is included in MP or SE and whether it is for a server or client documentation.

@klustria klustria requested a review from tjquinno July 23, 2022 09:11
Copy link
Member

@tjquinno tjquinno left a comment

Choose a reason for hiding this comment

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

A very few important fixes, then some maybe optional ones. The text is still using both "gRPC Server" and "gRPC server" (same for Client/client) in some places. I think using lower case for server and client throughout (except of course in headings) is preferable but it's minor.

There are several places that use relative cross-references. I mentioned some of them explicitly but there are others. It's not urgent to change them now, but "at our leisure" we should convert them to use the rootdir attribute instead.

docs/mp/grpc/client.adoc Outdated Show resolved Hide resolved
docs/mp/grpc/server.adoc Outdated Show resolved Hide resolved
docs/mp/grpc/server.adoc Outdated Show resolved Hide resolved
docs/mp/grpc/server.adoc Outdated Show resolved Hide resolved
docs/se/grpc/client.adoc Outdated Show resolved Hide resolved
docs/includes/grpc-marshalling.adoc Outdated Show resolved Hide resolved
docs/includes/grpc-marshalling.adoc Outdated Show resolved Hide resolved
@klustria
Copy link
Member Author

Made changes to replace "gRPC Server" with "gRPC server" and "gRPC Client" and "gRPC client" across all docs. However I left those that are in the links used in the example section because they are all first letter in the word capitalized to symbolize a title. Let me know if you think otherwise.

@klustria klustria requested a review from tjquinno July 23, 2022 17:05
@klustria klustria merged commit b264854 into helidon-io:master Jul 23, 2022
@klustria
Copy link
Member Author

This is additional updates for #4211 and #4316

Copy link
Contributor

@ljamen ljamen left a comment

Choose a reason for hiding this comment

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

Changes look good to me, thank you.

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