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

fix(runner): add suggested edit text from linter in display issue text #2135

Conversation

ifaisalalam
Copy link

The tool did not print suggested edits by the linter when displaying
the issues. If there is suggested edits by the linter, it should
be displayed along with the issue.

Closes #2134

@boring-cyborg
Copy link

boring-cyborg bot commented Jul 28, 2021

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Jul 28, 2021

CLA assistant check
All committers have signed the CLA.

@SVilgelm
Copy link
Member

Could you please show the examples of printed suggestions?

pkg/golinters/goanalysis/runners.go Outdated Show resolved Hide resolved
@ifaisalalam ifaisalalam force-pushed the ifaisalalam/fix/display-suggested-fix branch from 5b7cebe to 4519931 Compare July 28, 2021 17:49
@ifaisalalam
Copy link
Author

Could you please show the examples of printed suggestions?

Sure. Attaching screenshot of the displayed error after fix.

Screenshot 2021-07-28 at 11 20 34 PM

@ifaisalalam ifaisalalam requested a review from SVilgelm July 28, 2021 17:52
@SVilgelm
Copy link
Member

@ldez what do you think about this as temporary solution, it is really needed?

@ifaisalalam ifaisalalam force-pushed the ifaisalalam/fix/display-suggested-fix branch from 4519931 to b527a26 Compare July 28, 2021 17:55
@SVilgelm
Copy link
Member

@ifaisalalam please also run using golangci-lint run --out-format=github-actions and attach a screen shot

@ifaisalalam
Copy link
Author

golangci-lint run --out-format=github-actions

Sure, here it is.

Screenshot 2021-07-28 at 11 27 30 PM

@ldez ldez self-requested a review July 28, 2021 17:58
@ldez ldez added the enhancement New feature or improvement label Jul 28, 2021
pkg/golinters/goanalysis/runners.go Outdated Show resolved Hide resolved
@ldez
Copy link
Member

ldez commented Jul 28, 2021

For me, it's the same idea as #1778

@SVilgelm
Copy link
Member

yes, but the implementation is simpler

@ifaisalalam ifaisalalam force-pushed the ifaisalalam/fix/display-suggested-fix branch from b527a26 to 27eaa51 Compare July 28, 2021 18:06
@ifaisalalam
Copy link
Author

@SVilgelm Output after the latest changes.

Screenshot 2021-07-28 at 11 35 45 PM

Screenshot 2021-07-28 at 11 35 56 PM

@ifaisalalam ifaisalalam requested a review from SVilgelm July 28, 2021 18:10
pkg/golinters/goanalysis/runners.go Outdated Show resolved Hide resolved
@ldez ldez added the blocked Need's direct action from maintainer label Jul 28, 2021
@ldez
Copy link
Member

ldez commented Jul 28, 2021

IMO, this approach is not viable:

  • the markdown code fences syntax is not adapted
  • the output uses to write the suggestion is also not adapted

I'm still convinced that the real solution is to add support for SuggestedFixes (#1779) to replace the old system.

I'm still working on it but, for me, it's a part of the v2.

I'll need to take time to define a v2 plan.

The tool did not print suggested edits by the linter when displaying
the issues. If there is suggested edits by the linter, it should
be displayed along with the issue.

Closes golangci#2134
@ifaisalalam ifaisalalam force-pushed the ifaisalalam/fix/display-suggested-fix branch from 27eaa51 to 34d4b45 Compare July 29, 2021 08:48
@ifaisalalam
Copy link
Author

@SVilgelm @ldez I am working on the fix using the approach we discussed yesterday.

Can you please review and share any feedback on my approach so far?

@ifaisalalam ifaisalalam requested a review from SVilgelm July 29, 2021 09:35
@ifaisalalam ifaisalalam force-pushed the ifaisalalam/fix/display-suggested-fix branch from e437bcb to abc3f26 Compare July 29, 2021 10:40
@ifaisalalam ifaisalalam force-pushed the ifaisalalam/fix/display-suggested-fix branch from d7c4b11 to 8a08c5e Compare July 29, 2021 11:26
@ifaisalalam
Copy link
Author

ifaisalalam commented Jul 29, 2021

Here are the outputs from the printers.

HTML

With suggested edits
Screenshot 2021-07-29 at 2 44 41 PM
Without suggested edit
Screenshot 2021-07-29 at 2 56 38 PM

JSON

{
  "Issues": [
    {
      "FromLinter": "govet",
      "Text": "fieldalignment: struct with 80 pointer bytes could be 64",
      "SuggestedFixes": [
        {
          "Message": "Rearrange fields",
          "TextEdits": [
            {
              "Pos": 46401357,
              "End": 46401522,
              "NewText": "struct {\n\tvalidator Validator\n\tfilter    map[string]interface{}\n\tcontext   map[string]interface{}\n\trelations map[string]Query\n\tName      string\n\tfields    []string\n}"
            }
          ]
        }
      ],
      "Severity": "",
      "SourceLines": [
        "type query struct {"
      ],
      "Replacement": null,
      "Pos": {
        "Filename": "query/query.go",
        "Offset": 517,
        "Line": 23,
        "Column": 12
      },
      "ExpectNoLint": false,
      "ExpectedNoLintLinter": ""
    }
  ],
  "Report": {
    "Linters": [
      {
        "Name": "govet",
        "Enabled": true,
        "EnabledByDefault": true
      },
      {
        "Name": "bodyclose"
      },
      {
        "Name": "noctx"
      },
      {
        "Name": "errcheck",
        "Enabled": true,
        "EnabledByDefault": true
      },
      {
        "Name": "golint"
      },
      {
        "Name": "rowserrcheck"
      },
      {
        "Name": "staticcheck",
        "Enabled": true,
        "EnabledByDefault": true
      },
      {
        "Name": "unused",
        "EnabledByDefault": true
      },
      {
        "Name": "gosimple",
        "Enabled": true,
        "EnabledByDefault": true
      },
      {
        "Name": "stylecheck"
      },
      {
        "Name": "gosec"
      },
      {
        "Name": "structcheck",
        "Enabled": true,
        "EnabledByDefault": true
      },
      {
        "Name": "varcheck",
        "EnabledByDefault": true
      },
      {
        "Name": "interfacer"
      },
      {
        "Name": "unconvert"
      },
      {
        "Name": "ineffassign",
        "Enabled": true,
        "EnabledByDefault": true
      },
      {
        "Name": "dupl"
      },
      {
        "Name": "goconst"
      },
      {
        "Name": "deadcode",
        "Enabled": true,
        "EnabledByDefault": true
      },
      {
        "Name": "gocyclo"
      },
      {
        "Name": "cyclop"
      },
      {
        "Name": "gocognit"
      },
      {
        "Name": "typecheck",
        "EnabledByDefault": true
      },
      {
        "Name": "asciicheck"
      },
      {
        "Name": "gofmt"
      },
      {
        "Name": "gofumpt"
      },
      {
        "Name": "goimports",
        "Enabled": true
      },
      {
        "Name": "goheader"
      },
      {
        "Name": "gci"
      },
      {
        "Name": "maligned"
      },
      {
        "Name": "depguard"
      },
      {
        "Name": "misspell"
      },
      {
        "Name": "lll"
      },
      {
        "Name": "unparam"
      },
      {
        "Name": "dogsled"
      },
      {
        "Name": "nakedret"
      },
      {
        "Name": "prealloc"
      },
      {
        "Name": "scopelint"
      },
      {
        "Name": "gocritic"
      },
      {
        "Name": "gochecknoinits"
      },
      {
        "Name": "gochecknoglobals"
      },
      {
        "Name": "godox"
      },
      {
        "Name": "funlen"
      },
      {
        "Name": "whitespace"
      },
      {
        "Name": "wsl"
      },
      {
        "Name": "goprintffuncname"
      },
      {
        "Name": "gomnd"
      },
      {
        "Name": "goerr113"
      },
      {
        "Name": "gomodguard"
      },
      {
        "Name": "godot"
      },
      {
        "Name": "testpackage"
      },
      {
        "Name": "nestif"
      },
      {
        "Name": "exportloopref"
      },
      {
        "Name": "exhaustive"
      },
      {
        "Name": "sqlclosecheck"
      },
      {
        "Name": "nlreturn"
      },
      {
        "Name": "wrapcheck"
      },
      {
        "Name": "thelper"
      },
      {
        "Name": "tparallel"
      },
      {
        "Name": "exhaustivestruct"
      },
      {
        "Name": "errorlint"
      },
      {
        "Name": "paralleltest"
      },
      {
        "Name": "makezero"
      },
      {
        "Name": "forbidigo"
      },
      {
        "Name": "ifshort"
      },
      {
        "Name": "predeclared"
      },
      {
        "Name": "revive",
        "Enabled": true
      },
      {
        "Name": "durationcheck"
      },
      {
        "Name": "wastedassign"
      },
      {
        "Name": "importas"
      },
      {
        "Name": "nilerr"
      },
      {
        "Name": "forcetypeassert"
      },
      {
        "Name": "gomoddirectives"
      },
      {
        "Name": "promlinter"
      },
      {
        "Name": "tagliatelle"
      },
      {
        "Name": "nolintlint"
      }
    ]
  }
}

Text

Screenshot 2021-07-29 at 5 06 53 PM

Tab

Screenshot 2021-07-29 at 5 07 35 PM

JUnit XML

Screenshot 2021-07-29 at 5 09 43 PM

CodeClimate

[
  {
    "description": "govet: fieldalignment: struct with 80 pointer bytes could be 64",
    "content": "Rearrange fields\n```\nstruct {\n\tvalidator Validator\n\tfilter    map[string]interface{}\n\tcontext   map[string]interface{}\n\trelations map[string]Query\n\tName      string\n\tfields    []string\n}\n```\n",
    "fingerprint": "82010A31125F82A5AD347C003E45E6D5",
    "location": {
      "path": "query/query.go",
      "lines": {
        "begin": 23
      }
    }
  },
  {
    "description": "govet: fieldalignment: struct with 464 pointer bytes could be 400",
    "content": "Rearrange fields\n```\nstruct {\n\tLastName   string  `json:\"last_name,omitempty\" type:\"field\"`\n\tFullName   string  `json:\"full_name,omitempty\" type:\"field\"`\n\tEmail      string  `json:\"email,omitempty\" type:\"field\"`\n\tDateJoined string  `json:\"date_joined,omitempty\" type:\"field\"`\n\tFirstName  string  `json:\"first_name,omitempty\" type:\"field\"`\n\tUsername   string  `json:\"username,omitempty\" type:\"field\"`\n\tProfile    Profile `json:\"profile\" type:\"relation\"`\n\tUserID     int64   `json:\"user_id,omitempty\" type:\"field\"`\n\tID         int64   `json:\"id,omitempty\" type:\"field\"`\n}\n```\n",
    "fingerprint": "6CB0720E8248039688D9966B74111915",
    "location": {
      "path": "user/user.go",
      "lines": {
        "begin": 3
      }
    }
  }
]
---

GitHub and CheckStyle printers remain now.

Text string
FromLinter string
Text string
SuggestedFixes []SuggestedFix
Copy link
Author

@ifaisalalam ifaisalalam Jul 29, 2021

Choose a reason for hiding this comment

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

Using custom SuggestedFix struct because analysis.SuggestedFix.TextEdits[i].NewText is type []byte which is not serialized properly to JSON/XML.

@SuperQ
Copy link

SuperQ commented Nov 4, 2021

Any progress on getting this in? I tested it out and it was extremely useful.

@ldez
Copy link
Member

ldez commented Nov 4, 2021

No "visible" progress, but the support of suggested fixes is still an important topic:
#2135 (comment)

@ifaisalalam
Copy link
Author

Can we at least get this fix merged with only the text printer? This will help developers fix issues in their projects when developing locally.

I also think that maybe we can pass a flag via the command line or the config YAML file, and the suggestion will only be printed when this flag is enabled.

@ldez
Copy link
Member

ldez commented Dec 24, 2021

If we merge something we have to handle it, to maintain it, add we will have to fight again when we will want to create the right thing.
So I don't think there is something that can be merged without a cost.
The classic open-source approach is to add things that we don't need to remove after: "Rule n°1 of open-source: no is temporary, yes is forever."

I still agree with myself #2135 (comment)

@SVilgelm SVilgelm removed their request for review October 9, 2023 04:06
@SVilgelm
Copy link
Member

SVilgelm commented Oct 9, 2023

no changes in two years. close it

@SVilgelm SVilgelm closed this Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Need's direct action from maintainer enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggested Edits from Linters do not get printed
5 participants