-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
examples: Added README files for all missing Examples #11676
examples: Added README files for all missing Examples #11676
Conversation
@@ -135,6 +136,24 @@ before trying out the examples. | |||
|
|||
- [Keep Alive](src/main/java/io/grpc/examples/keepalive) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear to me which part of Sanjay Pujare's concerns this PR addresses.
"each example should have a README file which a user can read to understand the example." I don't find this done, for example there is no README for the subdirectories in https://github.com/grpc/grpc-java/tree/master/examples/src/main/java/io/grpc/examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kannanjgithub @shivaspeaks There is a common README file for all the examples (which are inside the path https://github.com/grpc/grpc-java/tree/master/examples/src/main/java/io/grpc/examples) as mentioned in the below link and updated some of the missing ones with recent changes as part of this PR.
Common README link in grpc-java - https://github.com/grpc/grpc-java/blob/master/examples/README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue mentions "each example should have a README file which a user can read to understand the example." Merely enumerating the examples in the sub-directories to the parent README does not do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see there are 21 examples, working on this comment and its in progress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kannanjgithub @shivaspeaks Added the Readme file with details for the all examples and its ready for Review. please have look once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kannanjgithub @shivaspeaks Added the Readme file with details for the all examples and its ready for Review. please have look once.
@kannanjgithub @shivaspeaks This PR is pending for review from long time, Request you have a look once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments.
I see this says |
@ejona86 Thanks for your reply, I was refering to one of the change as the "directory structure" for example-alts as it is not consistant with all the other examples projects directory so made this change/move as below in this PR apart from this no other changes related to directory structure. "GRPC Example documentation" was refering here only for adding new README files in all examples with explanations nothing else. |
Sigh... my comment #11676 (comment) still applies. This doesn't fix #11676 and the directory structure it was talking about it not related to this change. |
Oh, but I thought the need for those changes was striked out by your comment on #5467 (comment)
If not, perhaps we can reopen #5467 and edit the issue description with what's left. |
@shivaspeaks, I see that you improved the PR description, but that didn't get copied into the commit itself. That information should generally be in the commit; the only information I tend to omit in the commit message is messages to the reviewer (e.g., "This is the first part; I will send out the second part next week.") Managing that is much easier for your own PRs as you can write a good commit message and have github auto-copy it to the PR description. But if the review changes the code substantially or when you merge someone else's change, it is good to double-check the commit message. I do think the example structure could be improved, but #5467 didn't really say what was wrong or what would be better and it is low-quality, mixing of multiple things, and confusing enough that a replacement issue would maybe be better. FWIW, that needed discussion/design for what would be better. The issue was "this needs improving" and I agree with that, but that doesn't mean the solution is fully-formed. (There are some examples, like the json one, that should be moved to its own directory because they have extra dependencies. But what changes to do where really requires an audit to look through it and see what could be improved.) |
Added new README files for all the examples with details and changed the incorrectness/ typo in path of
examples/example-alts/example-alts/README.md
→examples/example-alts/README.md
Fixes #5467