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: add period to end of generated comment #7392

Merged
merged 9 commits into from
Jul 12, 2024

Conversation

infovivek2020
Copy link
Contributor

@infovivek2020 infovivek2020 commented Jul 4, 2024

Addresses: #7350
RELEASE NOTES: N/A

Copy link

linux-foundation-easycla bot commented Jul 4, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.55%. Comparing base (ee62e56) to head (d9a3efc).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7392      +/-   ##
==========================================
+ Coverage   81.50%   81.55%   +0.05%     
==========================================
  Files         348      348              
  Lines       26752    26752              
==========================================
+ Hits        21804    21818      +14     
+ Misses       3766     3757       -9     
+ Partials     1182     1177       -5     

see 12 files with indirect coverage changes

@infovivek2020
Copy link
Contributor Author

infovivek2020 commented Jul 4, 2024

@purnesh42H previous PR #7362 do consider this one
Do assign me as assignee

@purnesh42H purnesh42H self-assigned this Jul 5, 2024
@purnesh42H purnesh42H self-requested a review July 5, 2024 04:09
@purnesh42H purnesh42H added the Type: Documentation Documentation or examples label Jul 5, 2024
@purnesh42H
Copy link
Contributor

Thanks @infovivek2020. LGTM. Please take a look at some of the other submitted PRs and add RELEASE NOTES section in your PR description as part of Mergeable check

@purnesh42H
Copy link
Contributor

@infovivek2020 can you comment on the issue #7350 that you have raised a PR for this so that I can assign it to you

@purnesh42H
Copy link
Contributor

@infovivek2020 just a reminder to add RELEASE NOTES in the PR description so that we can go ahead and merge

@purnesh42H
Copy link
Contributor

@infovivek2020 don't edit changelog.md. In your PR description, you can mention "RELEASE NOTES: N/A"

@easwars
Copy link
Contributor

easwars commented Jul 9, 2024

@infovivek2020 : Could you please sign the CLA.

@dfawley dfawley changed the title comment is added to the generated protoc gen protoc-gen-go-grpc: add period to end of generated comment Jul 9, 2024
@infovivek2020
Copy link
Contributor Author

@purnesh42H right now i am facing "HEAD is now at 0dfcf4e Merge c26c5bd into daab563
Error: Process completed with exit code 1."

@purnesh42H
Copy link
Contributor

@arvindbr8 is the vet issue fixed? Do we have to regenerate the protos again?

@dfawley
Copy link
Member

dfawley commented Jul 10, 2024

@arvindbr8 is the vet issue fixed? Do we have to regenerate the protos again?

This PR affects codegen, so it should regenerate everything regardless.

@arvindbr8
Copy link
Member

@arvindbr8 is the vet issue fixed? Do we have to regenerate the protos again?

Not very important, but relevant for this PR.

This #7351 introduce support for editions. However our regenerate scripts is broken complaining something like this:

protoc-gen-go: invalid FileDescriptorProto "examples/route_guide/routeguide/route_guide.proto": proto: invalid syntax: "editions"

I'm investigating this. Let me see if I get some time to fix this today.

@purnesh42H
Copy link
Contributor

@arvindbr8 is the vet issue fixed? Do we have to regenerate the protos again?

This PR affects codegen, so it should regenerate everything regardless.

yeah I mean the protos are regenerated but do we have to regenerate again after Arvind's fix?

@purnesh42H
Copy link
Contributor

@infovivek2020 can you pull and rebase your branch on latest upstream and then run ./scripts/regenerate.sh again?

@purnesh42H
Copy link
Contributor

purnesh42H commented Jul 11, 2024

@infovivek2020 looks like you got unwanted files in your push. Can you try following

  1. git checkout master
  2. git pull upstream master
  3. git rebase upstream/master
  4. git checkout "your branch"
  5. git rebase master
  6. git push --force

@purnesh42H
Copy link
Contributor

@dfawley this looks good to be merged

@dfawley dfawley merged commit e7d8822 into grpc:master Jul 12, 2024
13 checks passed
printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024
printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 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.

5 participants