chore: enable int-conversion from perfsprint #8194
Conversation
mmorel-35
requested review from
afdesk,
simar7,
nikpivkin,
knqyf263 and
DmitriyLewen
as code owners
mmorel-35
force-pushed
the
perfsprint/strconcat
branch
from
8d1f7a7 to
be6aebd
Compare
mmorel-35
changed the title
mmorel-35
force-pushed
the
perfsprint/strconcat
branch
from
f3c8f15 to
7cefcc7
Compare
pkg/cache/redis_test.go
Outdated
| @@ -66,7 +66,7 @@ func TestRedisCache_PutArtifact(t *testing.T) { | |||
| addr = "dummy:16379" | |||
| } | |||
|
|
|||
| c, err := cache.NewRedisCache(fmt.Sprintf("redis://%s", addr), "", "", "", false, 0) | |||
| c, err := cache.NewRedisCache("redis://"+addr, "", "", "", false, 0) | |||
There was a problem hiding this comment.
I feel like it's too much. `fmt.Sprintf is easier to read than string concatenation, even with a performance downside. However, if it drastically improves performance, it's worth considering.
.golangci.yaml
Outdated
| # Optimizes into strings concatenation. | ||
| strconcat: false | ||
| strconcat: true |
There was a problem hiding this comment.
I personally prefer not to use this optimization.
| strconcat: true | |
| strconcat: false |
@DmitriyLewen @simar7 @nikpivkin However, if you prefer string concatenation, I'm okay with that.
There was a problem hiding this comment.
I would also vote not to use this. fmt.Sprintf seems fine to me. Is this subjective or there's something on the table w.r.t. performance by using this?
There was a problem hiding this comment.
sometimes we use a long strings, so string concatenation will be almost unreadable.
So I think it is better to use fmt.Sprintf.
There was a problem hiding this comment.
I think the performance gain will be imperceptible, but the readability will be lost. If we find a weakness, we can improve it.
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
mmorel-35
force-pushed
the
perfsprint/strconcat
branch
from
7cefcc7 to
3540ec8
Compare
mmorel-35
changed the title
| @@ -132,7 +133,7 @@ func parseTimestamp(ts string) (time.Time, error) { | |||
| case "Z07:00": | |||
| what = "UTC offset" | |||
| case "T": | |||
| return time.Time{}, fmt.Errorf("not a valid RFC3339 timestamp: missing required time introducer 'T'") | |||
| return time.Time{}, errors.New("not a valid RFC3339 timestamp: missing required time introducer 'T'") | |||
There was a problem hiding this comment.
nit:
| return time.Time{}, errors.New("not a valid RFC3339 timestamp: missing required time introducer 'T'") | |
| return time.Time{}, xerrors.New("not a valid RFC3339 timestamp: missing required time introducer 'T'") |
Description
enable and fixes all rules from perfsprint linter