Test-infra: /lint should suggest changes
how cool what that be?
/kind enhancement
/area prow
/area prow/plugins
/priority backlog
/help
if anyone wants to look at building something like this, it would make an awesome prow plugin
BenTheElder
All 20 comments
@BenTheElder what are the requirements for this issue? I was trying to see an example output of /lint and found this PR: #9644 and saw that /lint did suggest changes:
taragu
on 21 Oct 2018
Er this would be a bit more involved, github now allows suggesting what the code should be, which the user can accept and get a new commit with the changes applied.
https://blog.github.com/2018-10-16-future-of-software/#suggested-changes-public-beta
It would be cool if we automatically suggested code fixes using this :-)
BenTheElder
on 22 Oct 2018
@BenTheElder ah i see. Thanks for the clarification! I'd like to work on this item
taragu
on 25 Oct 2018
Have at it! I think it will be tricky to wire up something to make accurate suggestions, but this feature could be really nice :-)
BenTheElder
on 25 Oct 2018
@BenTheElder I created a test case and a rough implementation for one use case. Could you please take a look to see if I'm on the right track before I continue implementing the rest of the cases? Thank you so much in advance!
https://github.com/kubernetes/test-infra/compare/master...taragu:lint-suggest-changes?expand=1
taragu
on 3 Nov 2018
@taragu :tada: thanks for sending a PR!
Just saw this and thought I'd mention it here: https://twitter.com/fatih/status/1059888289979813889 is a tool which does pretty much this using the "suggest changes" feature.
https://fixmie.com/ for more details.
nikhita
on 7 Nov 2018
@nikhita ahh thanks for the info! Looks like we shouldn't reinvent the wheel. Is there an internal approval process for using this tool?
taragu
on 7 Nov 2018
/remove-help
matthyx
on 10 Nov 2018
IMO this is still something nice to build into Prow, we have room to do other things with the suggestions functionality but this is the first obvious step to use it imo. Leveraging the new suggest changes functionality in Prow was the intention behind opening this issue.
BenTheElder
on 13 Nov 2018
Does this make sense for e.g. rename-type issues? AFAIK, the linter is only going to suggest changes at the declaration, but not for any uses — so for the suggestions to be meaningful, you need to use GitHub to apply the changes, pull the results, fix your code, push the results, _and_ then later squash your commits. You may even need to actually undo the suggestion in order to use your editor's refactor functionality to perform the rename.
I think to be useful this really needs to either be sufficiently smart to also fix usages, or only target changes that aren't going to require more changes elsewhere.
Katharine
on 25 Nov 2018
/assign
taragu
on 27 Jan 2019
The last use case left is package comments. For these, the line number of lint.Problem is different than the actual line number of the problem location. An example problem would be "package comment should not have leading space"
/*
Package foo is pretty sweet.
*/
package foo
The line number p.Problem.Line and the line text p.LineText points to /*. So this doesn't really fit in with the existing implementation of golint_suggestion.go, and I haven't thought of a good way to handle this.
@cjwagner @stevekuznetsov @bentheelder @fejta any thoughts?
taragu
on 27 Jan 2019
If we're just pasting string suggestions and not using GItHub's beta review suggestion tool I think just pasting it to the line that the linter picks up is fine
stevekuznetsov
on 27 Jan 2019
If we're just pasting string suggestions and not using GItHub's beta review suggestion tool I think just pasting it to the line that the linter picks up is fine
We are using GitHub's review suggestion feature, not plain string suggestions.
cjwagner
on 28 Jan 2019
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale
fejta-bot
on 30 Apr 2019
Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
fejta-bot
on 30 May 2019
@taragu is this still being worked on? Should we close that?
matthyx
on 1 Jun 2019
@matthyx no this is not being worked on. If there's no objection we can close this issue
taragu
on 1 Jun 2019
I had the impression this was the umbrella issue for all your great changes regarding lint.
Thanks a lot :)
I will let @BenTheElder reopen it, if needed.
/close
matthyx
on 1 Jun 2019
@matthyx: Closing this issue.
In response to this:
I had the impression this was the umbrella issue for all your great changes regarding lint.
Thanks a lot :)
I will let @BenTheElder reopen it, if needed.
/close
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
k8s-ci-robot
on 1 Jun 2019
Related issues
benmoss
·
3Comments
MrHohn
·
4Comments
spiffxp
·
3Comments
stevekuznetsov
·
4Comments
BenTheElder
·
4Comments