[#10] linters: add useStrconv linter #10

achuprov · 2023-08-15T15:03:56Z

achuprov commented

2023-08-15 15:03:56 +00:00

Added a linter that suggests using strconv instead of fmt.

Signed-off-by: Alexander Chuprov a.chuprov@yadro.com

Added a linter that suggests using strconv instead of fmt. Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>

achuprov changed title from ~~[#10] linters: add nofmt linter~~ to WIP: [#10] linters: add nofmt linter

2023-08-15 15:04:01 +00:00

achuprov force-pushed nofmt from 200519f3cd to 61fab5ddd8

2023-08-16 11:53:39 +00:00

Compare

achuprov force-pushed nofmt from 61fab5ddd8 to 4ebbf63eb9

2023-08-16 11:56:17 +00:00

Compare

achuprov changed title from ~~WIP: [#10] linters: add nofmt linter~~ to [#10] linters: add nofmt linter

2023-08-16 11:58:33 +00:00

achuprov requested review from storage-core-developers 2023-08-16 11:58:52 +00:00

achuprov requested review from storage-core-committers 2023-08-16 11:58:53 +00:00

dstepanov-yadro requested changes 2023-08-16 12:17:51 +00:00

README.md Outdated

					
				@ -49,3 +51,3 @@

				linters-settings:

				    custom:

				        noliteral:

				        custom-linter:

dstepanov-yadro commented

2023-08-16 12:12:35 +00:00

Is such name mandatory? Maybe truecloudlab or frostfs is better?

Is such name mandatory? Maybe `truecloudlab` or `frostfs` is better?

dstepanov-yadro marked this conversation as resolved

internal/analyzers/nofmt/linter.go Outdated

					
				@ -0,0 +34,4 @@

					Methods: []Method{

						{

							TargetMethods:       "Sprintf",

							Modyficators:        []string{"%d", "%f", "%t", "%x"},

dstepanov-yadro commented

2023-08-16 12:15:52 +00:00

targetMethods, modificators, positionModificator and package are implementation details, not config parameters. I think there should be no parameters at all for this check.

dstepanov-yadro marked this conversation as resolved

internal/analyzers/nofmt/linter.go Outdated

					
				@ -0,0 +42,4 @@

					Enable: true,

				}

				func ConfigurationLinter(conf any) (*analysis.Analyzer, error) {

dstepanov-yadro commented

2023-08-16 12:11:17 +00:00

Since this is a constructor, it is better to use New as the name of the method.

Since this is a constructor, it is better to use `New` as the name of the method.

dstepanov-yadro marked this conversation as resolved

main.go Outdated

					
				@ -7,3 +6,2 @@

					noliteral "git.frostfs.info/TrueCloudLab/linters/internal/analyzers/noliteral"

					"github.com/mitchellh/mapstructure"

					"golang.org/x/tools/go/analysis"

dstepanov-yadro commented

2023-08-16 12:09:23 +00:00

Our projects usually use two import groups.

dstepanov-yadro marked this conversation as resolved

pkg/util/ast.go Outdated

					
				@ -0,0 +1,96 @@

				package util

dstepanov-yadro commented

2023-08-16 12:06:04 +00:00

There is such an expression: util classes (packages, packets etc) are evil. util is the right place for everything.

For example, ast package is more specific.

There is such an expression: `util classes (packages, packets etc) are evil`. `util` is the right place for everything. For example, `ast` package is more specific.

achuprov force-pushed nofmt from 4ebbf63eb9 to 003ae9ae11

2023-08-16 13:10:57 +00:00

Compare

achuprov force-pushed nofmt from 003ae9ae11 to 69dab19c19

2023-08-16 13:20:32 +00:00

Compare

achuprov commented

2023-08-16 13:22:11 +00:00

@dstepanov-yadro fixed

dstepanov-yadro requested changes 2023-08-16 13:52:58 +00:00

README.md Outdated

					
				@ -57,2 +57,2 @@

				                constants-package: "git.frostfs.info/rep/logs" # if not set, then the check is disabled

				                noliteral:

				                    enable: true # required

dstepanov-yadro commented

2023-08-16 13:42:25 +00:00

Is it really required? As is see, if you do not pass this value, the linter just will be turned off.

I suggest to correct the comment, for example if not defined, then noliteral check will be disabled

Is it really required? As is see, if you do not pass this value, the linter just will be turned off. I suggest to correct the comment, for example `if not defined, then noliteral check will be disabled`

dstepanov-yadro marked this conversation as resolved

internal/analyzers/nofmt/linter.go Outdated

					
				@ -0,0 +28,4 @@

				var modyficators = []string{"%d", "%f", "%t", "%x"}

				func New(conf any) (*analysis.Analyzer, error) {

					configMap, ok := conf.(map[string]any)

dstepanov-yadro commented

2023-08-16 13:50:09 +00:00

What happens if there is no section for this linter in the configuration file?

I expect the check should work in this case.

What happens if there is no section for this linter in the configuration file? I expect the check should work in this case.

dstepanov-yadro marked this conversation as resolved

internal/analyzers/noliteral/linter.go Outdated

					
				@ -75,3 +89,3 @@

							})

							return false

							return true

dstepanov-yadro commented

2023-08-16 13:48:08 +00:00

Please explain this change.

dstepanov-yadro marked this conversation as resolved

achuprov force-pushed nofmt from 69dab19c19 to c049dbe1c8

2023-08-16 14:28:42 +00:00

Compare

achuprov force-pushed nofmt from c049dbe1c8 to 63e20aedd0

2023-08-16 14:46:34 +00:00

Compare

dstepanov-yadro approved these changes 2023-08-16 14:55:46 +00:00

achuprov referenced this pull request from TrueCloudLab/frostfs-node

2023-08-16 15:35:47 +00:00

Add useStrconv linter #618

fyrchik reviewed 2023-08-17 09:01:22 +00:00

README.md Outdated

					
				@ -39,0 +36,4 @@

				| Name                    | Description                                                                                                                   |

				|-------------------------|-------------------------------------------------------------------------------------------------------------------------------|

				| [noliteral](#noliteral) | The tool prohibits the use of literal string arguments in logging functions.                                                   |

				| [nofmt](#nofmt)         | The `nofmt` linter recommends the utilization of `strconv` over `fmt` when performing string conversions of primitive data types. Detailed guidelines can be found in the [official Uber Go Style Guide](https://github.com/uber-go/guide/blob/master/style.md#prefer-strconv-over-fmt). |

fyrchik commented

2023-08-17 08:51:20 +00:00

nofmt is too generic (we have go fmt), maybe strconv?

`nofmt` is too generic (we have `go fmt`), maybe `strconv`?

README.md Outdated

					
				@ -59,3 +62,3 @@

				```

				##### ENV Configuration

				### nofmt

fyrchik commented

2023-08-17 08:52:04 +00:00

I think noliteral may also have a separate section.

achuprov commented

2023-08-17 13:40:24 +00:00

noliteral already has its own section in the README.md d0450b6301/README.md (noliteral)

`noliteral` already has its own section in the README.md https://git.frostfs.info/TrueCloudLab/linters/src/commit/d0450b63015359a704073c6d85559082e6f54d92/README.md#noliteral

internal/analyzers/nofmt/linter.go Outdated

					
				@ -0,0 +76,4 @@

								return true

							}

							modValue := strings.Replace(stringLiteral.Value, "\"", "", -1)

fyrchik commented

2023-08-17 09:00:49 +00:00

Let's add a positive case (no report) for

fmt.Sprint(`"%d"`)

We should remove a single character from the beginning and from the end. Can we use either slices ([1:len()-1]) or

func unquote(s string) string {
  var res string
  fmt.Sscanf(a, "%q", &res)
  return res
}

1. Let's add a positive case (no report) for ```golang fmt.Sprint(`"%d"`) ``` 2. We should remove a single character from the beginning and from the end. Can we use either slices ([1:len()-1]) or ```golang func unquote(s string) string { var res string fmt.Sscanf(a, "%q", &res) return res } ```