strconv.ParseInt(myID, 10, 32) --> add-constant: avoid magic numbers like '10'

[guettli]

trafficstars

Is your feature request related to a problem? Please describe.

I get this warning:

foo/bar.go:53:94: add-constant: avoid magic numbers like '10', create a named constant for it (revive)
        deviceID, err := strconv.ParseInt(myID, 10, 32)

I see these ways to handle this:

• add a nolint-comment
• creating a constant
• configure the linter

All these solutions are extra work and not nice.

Describe the solution you'd like

I think the above warning should not appear by default.

Describe alternatives you've considered

I can solve this on my own, but a solution for everybody would be better.

Additional context

I use:

golangci-lint has version 1.50.1 built from 8926a95f on 2022-10-22T10:50:47Z

[chavacava]

Hi @guettli, thanks for filling the issue. IMO it's a matter of taste. For example, I prefer

strconv.ParseInt(myID, asDecimal, of32bitsSize)

We could wait some feedback from other users to see if there is some kind of consensus on the subject (and, if necessary, in the way to implement a solution)

[guettli]

@chavacava until you got enough feedback from the community: What is the best way to silence this particular check?

[chavacava]

For this particular rule you can:

1. Add a //revive:disable:add-constant at each occurrence of false positives, or
2. Add 10 (and 32?) to the allowInts configuration parameter of the rule, or
3. Add strconv\\.ParseInt to the ignoreFuncs configuration parameter of the rule

(you can find an example of the rule's configuration here)

Side note: I do not activate this rule in my setup mainly because its added value does not worth the configuration effort.

[guettli]

@chavacava sorry, that I need to ask you again. I tried to ignore it via ignoreFuncs like this:

  revive:
    enable-all-rules: true
    rules:
      # https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md#add-constant
      - name: add-constant
        severity: warning
        disabled: false
        arguments:
          - maxLitCount: "3"
            allowStrs: '""'
            allowInts: "0,1,2"
            ignoreFuncs: "strconv\\.ParseInt"

But it fails.

Code:

deviceID, err := strconv.ParseInt(strings.TrimPrefix(node.Spec.ProviderID, providerPrefix), 10, 32)

The line gets ignored if I add 10 and 32 to allowInts.

[chavacava]

@guettli I will check from my side.

[chavacava]

Hi @guettli, sorry for the late response. I've tested configuring the rule to ignore strconv.ParseInt calls and it works as expected.

Source code:

package main

import (
	"strconv"
	"strings"
)

func test() {
	deviceID, err := strconv.ParseInt(strings.TrimPrefix(node.Spec.ProviderID, providerPrefix), 10, 32)
}

Configuration:

confidence = 0.1
enableAllRules = false
severity = "error"
errorCode = 1
warningCode = 1

[rule.add-constant]
  arguments = [{"ignoreFuncs"= "strconv\\.ParseInt"}]

Running the linter produces, as expected, no output:

$ ./revive -config deleteme2.toml deleteme.go 
$ 

If I modify the configuration to do not ignore calls to strconv.ParseInt

confidence = 0.1
enableAllRules = false
severity = "error"
errorCode = 1
warningCode = 1

[rule.add-constant]

The linter reports the issue:

$ ./revive -config deleteme2.toml deleteme.go 
deleteme.go:11:94: avoid magic numbers like '10', create a named constant for it
deleteme.go:11:98: avoid magic numbers like '32', create a named constant for it

You are using golangci-lint to configure and run revive... my guess is that the regular expression in the golangci-lint conf should be written differently (with more or less \s before the .). You can try: strconv\.ParseInt, strconv\\\.ParseInt, strconv\\\\.ParseInt

[comdiv]

Think good design - keep base + allow do what exactly required for some people in custom config for rule.

While we knows at revive level package and name of function or package+obj+method (as in unhandled-error rule) we can add something like this:

[rule.add-constant]
    functions = [
         { name="strconv.ParseInt", values=[ "10","16","32", "64"  ]   },
    ]

Where are several cases where hard-coded numbers and strings are more readable and understandable than moved to constants. But it's not by default

[chavacava]

Closing for the moment. If the rule is too noisy (too much false positives) in your codebases, I recommend to deactivate it.