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

integration: add static code analysis tool; #377

Merged

Conversation

ss89
Copy link
Contributor

@ss89 ss89 commented Dec 7, 2020

@selm0 what do you think about this?

@selm0
Copy link
Contributor

selm0 commented Dec 8, 2020

@selm0 what do you think about this?

I was thinking about going for this one: https://golangci-lint.run/ it has your tool integrated.
First step, leaving the config untouched as it sounds quite reasonable. What's your opinion?

@ss89
Copy link
Contributor Author

ss89 commented Dec 9, 2020

this is going to take some time, it doesn't really state where the errors are, just writes some names...

@applike-ss
Copy link
Contributor

applike-ss commented Dec 10, 2020

i have executed this locally and it spits out way too many false positives for example for "unused" properties in a struct.

That may be the case, when you just look at the struct itsself, but since we embed that given struct in other structs, it is in fact used.

EDIT: This is the bug: golangci/golangci-lint#826

EDIT 2: For the moment i would stay with the staticcheck tool

@ss89 ss89 force-pushed the integration-add-static-code-analysis branch from f0ddec9 to 8f6affc Compare December 10, 2020 06:32
@ss89
Copy link
Contributor Author

ss89 commented Dec 15, 2020

@selm0 mind approving this?

@j4k4
Copy link
Member

j4k4 commented Dec 15, 2020

i like the idea of integrating static code analysis..
plz let me have a look at the tools, too :)

@selm0
Copy link
Contributor

selm0 commented Dec 15, 2020

i have executed this locally and it spits out way too many false positives for example for "unused" properties in a struct.

That may be the case, when you just look at the struct itsself, but since we embed that given struct in other structs, it is in fact used.

EDIT: This is the bug: golangci/golangci-lint#826

EDIT 2: For the moment i would stay with the staticcheck tool

You could disable structcheck.
How does the integration with Goland work with staticcheck?

@ss89
Copy link
Contributor Author

ss89 commented Dec 15, 2020

in fact i didn't integrate it into my goland, but used the go linter plugin which basically is a wrapper around golangci-lint. That however was putting my macbooks down on its knees because it was too weak to handle it. Or in other words: i wouldn't be able to be productive on the machine ^^

@ss89
Copy link
Contributor Author

ss89 commented Dec 15, 2020

@j4k4 i guess you will let me know about your findings and i will wait for you then?

@j4k4
Copy link
Member

j4k4 commented Dec 15, 2020

you will be using https://golangci-lint.run/ now?

@ss89
Copy link
Contributor Author

ss89 commented Dec 17, 2020

@j4k4 j4k4 force-pushed the master branch 4 times, most recently from aef1085 to cf6274e Compare February 1, 2021 15:20
@ss89 ss89 force-pushed the integration-add-static-code-analysis branch 2 times, most recently from ff83ea3 to 5cc8366 Compare February 7, 2021 16:17
@ss89
Copy link
Contributor Author

ss89 commented Feb 7, 2021

@selm0 mind having a look again? would really like to see this one merged soonish. Also please have a very close look (maybe together with @j4k4?) about the funcs i removed. Not sure if these should've been used at a later point or if they are left overs from the past.

Copy link
Member

@j4k4 j4k4 left a comment

Choose a reason for hiding this comment

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

i like adding the static checks

Copy link
Contributor

@selm0 selm0 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ss89 ss89 force-pushed the integration-add-static-code-analysis branch from 5cc8366 to 688f6da Compare February 10, 2021 07:15
@ss89
Copy link
Contributor Author

ss89 commented Feb 10, 2021

@j4k4 i removed some stuff you added yesterday. Please take a look at my rebase fixes commit if that is ok with you.

@j4k4
Copy link
Member

j4k4 commented Feb 10, 2021

all good

@ss89 ss89 force-pushed the integration-add-static-code-analysis branch from d0d6a58 to 5aef503 Compare February 10, 2021 07:27
@applike-ss applike-ss merged commit b551403 into justtrackio:master Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants