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

Code Improvement - Error strings should not be capitalized #1488

Merged

Conversation

wgliang
Copy link
Member

@wgliang wgliang commented Jan 25, 2021

Code Improvement - Error strings should not be capitalized
https://github.com/golang/go/wiki/CodeReviewComments#error-strings

@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #1488 (4779467) into main (6b34050) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1488   +/-   ##
=====================================
  Coverage   79.0%   79.0%           
=====================================
  Files        127     127           
  Lines       6683    6683           
=====================================
  Hits        5280    5280           
  Misses      1148    1148           
  Partials     255     255           
Impacted Files Coverage Δ
exporters/otlp/otlphttp/driver.go 93.5% <100.0%> (ø)
metric/registry/registry.go 92.1% <100.0%> (ø)

@XSAM
Copy link
Member

XSAM commented Jan 25, 2021

Good catch. I found there are still many codes that have such issues. Should we also fix these parts?

e.g.,

return fmt.Errorf("Failed with HTTP status %s", response.Status)
}
}
return fmt.Errorf("Failed to send data to %s after %d tries", address, d.cfg.maxAttempts)

@dashpole dashpole added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 25, 2021
@wgliang
Copy link
Member Author

wgliang commented Jan 26, 2021

@XSAM Sure. I will fix the current errors and add the linter check to ensure that it will not continue to occur in the future.

Base automatically changed from master to main January 29, 2021 19:39
@wgliang wgliang force-pushed the error-strings-should-not-be-capitalized branch from 32817ca to b8c2957 Compare February 2, 2021 02:31
@wgliang
Copy link
Member Author

wgliang commented Feb 2, 2021

Good catch. I found there are still many codes that have such issues. Should we also fix these parts?

e.g.,

return fmt.Errorf("Failed with HTTP status %s", response.Status)
}
}
return fmt.Errorf("Failed to send data to %s after %d tries", address, d.cfg.maxAttempts)

Done.

@wgliang
Copy link
Member Author

wgliang commented Feb 2, 2021

/ping @XSAM and anyone who needs to review PRs.

Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

LGTM

@wgliang
Copy link
Member Author

wgliang commented Feb 4, 2021

Who can merge this PR?

@MrAlias MrAlias merged commit 38efc87 into open-telemetry:main Feb 4, 2021
@MrAlias MrAlias mentioned this pull request Feb 12, 2021
ldelossa pushed a commit to ldelossa/opentelemetry-go that referenced this pull request Mar 5, 2021
…metry#1488)

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants