-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
chore: enable all rules of perfsprint linter #6164
chore: enable all rules of perfsprint linter #6164
Conversation
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6164 +/- ##
=======================================
Coverage 96.47% 96.47%
=======================================
Files 354 354
Lines 20126 20126
=======================================
Hits 19417 19417
Misses 524 524
Partials 185 185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -138,11 +139,11 @@ func (e *bridgeFieldEncoder) AddTime(key string, value time.Time) { | |||
} | |||
|
|||
func (e *bridgeFieldEncoder) AddUint(key string, value uint) { | |||
e.pairs = append(e.pairs, attribute.String(key, fmt.Sprintf("%d", value))) | |||
e.pairs = append(e.pairs, attribute.String(key, strconv.FormatUint(uint64(value), 10))) |
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.
why is this better?
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.
This is more performant
} | ||
|
||
func (e *bridgeFieldEncoder) AddUint64(key string, value uint64) { | ||
e.pairs = append(e.pairs, attribute.String(key, fmt.Sprintf("%d", value))) | ||
e.pairs = append(e.pairs, attribute.String(key, strconv.FormatUint(value, 10))) |
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.
this looks way uglier than sprintf
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 understand, this is more performant
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test