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 missing colon to errorf messages to improve readability #5911

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented Jan 6, 2023

Mostly add missing colons but also address few other minor issues.

It's confusing to read an error message without a colon for nesting, so that was the main motivation for the change. Happened to me couple of times and I couldn't find the whole string anywhere in code, wasted some time on it.

RELEASE NOTES:

  • none

@ash2k
Copy link
Contributor Author

ash2k commented Jan 9, 2023

Tests pass locally for me, I think it's a flaky test. Cannot rerun the action - no permissions.

@arvindbr8 arvindbr8 self-assigned this Jan 9, 2023
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

Hey @ash2k

lgtm, pinging @easwars for another set of eyes.

Also what are the other minor issues that you addressed here?

@@ -118,9 +118,10 @@ func main() {
// If -secure_mode is not set, expose all services on -port with a regular
// gRPC server.
if !*secureMode {
lis, err := net.Listen("tcp4", fmt.Sprintf(":%d", *port))
addr := fmt.Sprintf(":%d", *port)
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe init addr above this block and nix line 145?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to preserve the "laziness" of formatting, if that's ok.

@arvindbr8 arvindbr8 requested a review from easwars January 9, 2023 17:27
@arvindbr8 arvindbr8 assigned easwars and unassigned arvindbr8 Jan 9, 2023
@arvindbr8 arvindbr8 added the Type: Internal Cleanup Refactors, etc label Jan 9, 2023
@arvindbr8 arvindbr8 added this to the 1.53 Release milestone Jan 9, 2023
@arvindbr8
Copy link
Member

I'm also seeing more errors which seems to have been formatted with the missing colon. Wondering if we should sweep the repo with a regex grep to help us locate all of them.

@arvindbr8 arvindbr8 changed the title Cleanup error formatting and logging *: add missing colon to errorf messages to improve readability Jan 9, 2023
@ash2k
Copy link
Contributor Author

ash2k commented Jan 9, 2023

That's what I did - regex, but also checked some function usage manually. I certainly may have missed some places - there is a lot of code.

Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Some minor nits. Thank you so much for doing this.

benchmark/client/main.go Outdated Show resolved Hide resolved
benchmark/server/main.go Outdated Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
examples/features/health/client/main.go Outdated Show resolved Hide resolved
interop/server/server.go Outdated Show resolved Hide resolved
service_config.go Outdated Show resolved Hide resolved
stress/metrics_client/main.go Outdated Show resolved Hide resolved
xds/internal/resolver/xds_resolver.go Outdated Show resolved Hide resolved
@easwars easwars merged commit 379a2f6 into grpc:master Jan 18, 2023
@easwars
Copy link
Contributor

easwars commented Jan 18, 2023

Thank you very much for your contribution !!

@ash2k ash2k deleted the cleanup-errors branch January 18, 2023 02:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants