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

protoc-gen-go-grpc: update README to clarify --go-grpc_opt #7825

Closed

Conversation

hanut19
Copy link
Contributor

@hanut19 hanut19 commented Nov 11, 2024

Address: #5651
RELEASE NOTES: None

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.98%. Comparing base (dcba136) to head (2032c3c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7825      +/-   ##
==========================================
+ Coverage   81.91%   81.98%   +0.07%     
==========================================
  Files         377      377              
  Lines       38065    38065              
==========================================
+ Hits        31180    31208      +28     
+ Misses       5581     5558      -23     
+ Partials     1304     1299       -5     

see 16 files with indirect coverage changes

@arjan-bal arjan-bal added the Area: Documentation Includes examples and docs. label Nov 12, 2024
@arjan-bal arjan-bal added this to the 1.69 Release milestone Nov 12, 2024
@arjan-bal arjan-bal added Type: Documentation Documentation or examples and removed Area: Documentation Includes examples and docs. labels Nov 12, 2024
@@ -17,6 +17,10 @@ E.g.:
protoc --go-grpc_out=. --go-grpc_opt=require_unimplemented_servers=false[,other options...] \
```

**Using of --go-grpc_out and --go-grpc_opt**
Copy link
Contributor

Choose a reason for hiding this comment

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

@hanut19 you don't have to document go-grpc_out and go-grpc_opt for this task

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done please check

@@ -17,6 +17,10 @@ E.g.:
protoc --go-grpc_out=. --go-grpc_opt=require_unimplemented_servers=false[,other options...] \
Copy link
Contributor

Choose a reason for hiding this comment

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

the issue ask to make this statement more readable since we have gotten multiple complaints to be confused by this

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this

 protoc --go-grpc_out=. --go-grpc_opt=require_unimplemented_servers=false \
        --go-grpc_opt=[other options...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done please check

Copy link
Contributor

Choose a reason for hiding this comment

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

this suggestion won't work. both flags shouldn't be same. I meant the other one to be --go_opt

@purnesh42H purnesh42H assigned hanut19 and unassigned purnesh42H Nov 16, 2024
@hanut19 hanut19 force-pushed the document-updation-proto-gen-go-grpc-readme branch from 9b4666d to 9f6e45a Compare November 18, 2024 11:19
@purnesh42H purnesh42H changed the title docs: update documentation for confusion between --go-grpc_out and --go-grpc_opt in README protoc-gen-go-grpc: update README to clarify --go-grpc_opt Nov 19, 2024
@purnesh42H purnesh42H self-requested a review November 19, 2024 18:14
@@ -14,7 +14,8 @@ To restore this behavior, set the option `require_unimplemented_servers=false`.
E.g.:

```sh
protoc --go-grpc_out=. --go-grpc_opt=require_unimplemented_servers=false[,other options...] \
protoc --go-grpc_out=. --go-grpc_opt=require_unimplemented_servers=false \
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done please check

@@ -14,7 +14,8 @@ To restore this behavior, set the option `require_unimplemented_servers=false`.
E.g.:

```sh
protoc --go-grpc_out=. --go-grpc_opt=require_unimplemented_servers=false[,other options...] \
protoc --go-grpc_out=. --go-grpc_opt=require_unimplemented_servers=false \
--go-grpc_opt=[other options...]
Copy link
Contributor

Choose a reason for hiding this comment

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

need \ at the end and everything can be in single line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done please check

@hanut19 hanut19 force-pushed the document-updation-proto-gen-go-grpc-readme branch from 9f6e45a to 2032c3c Compare November 26, 2024 04:04
@purnesh42H purnesh42H assigned purnesh42H and unassigned hanut19 Nov 26, 2024
@purnesh42H purnesh42H self-requested a review November 26, 2024 05:55
@purnesh42H purnesh42H assigned hanut19 and unassigned purnesh42H Nov 26, 2024
@@ -14,7 +14,8 @@ To restore this behavior, set the option `require_unimplemented_servers=false`.
E.g.:

```sh
protoc --go-grpc_out=. --go-grpc_opt=require_unimplemented_servers=false[,other options...] \
protoc --go-grpc_out=. --go-grpc_opt=require_unimplemented_servers=false \
Copy link
Contributor

Choose a reason for hiding this comment

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

@hanut19 this is not correct. Did you try running this?

this is correct actually protoc --go-grpc_out=. --go-grpc_opt=require_unimplemented_servers=false[,other options...] \. It was fixed by this PR #6349

@purnesh42H
Copy link
Contributor

@hanut19 i will close this PR for now since it is actually fixed. I have asked Doug in the issue if we should actually delete this section. If yes, you can open a separate PR to delete the section.

@purnesh42H purnesh42H closed this Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Documentation or examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants