Closed c9845 closed 1 year ago
Patch coverage: 100.00%
and no project coverage change.
Comparison is base (
d54f22b
) 100.00% compared to head (7868d1a
) 100.00%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for opening this PR - it still seems to be WIP - ping me when you're ready for review or feedback.
@oliver006 I am done with this for now.
The questions becomes, do we want to implement staticcheck
for the repo. If yes, then this becomes a much larger change.
Thanks for clarifying. Leaving commented out code in the repo is not a good practice, dead code should just be removed unless there's a very good reason. The rest of the changes look alright. If you could clean up the commented-out code stuff then I think we should e good to go.
@oliver006 Commented out code has been removed.
What are your thoughts on utilizing staticcheck
?
Thanks @c9845 - this should be good to go.
I don't think there's an urgent need for adding staticcheck
to the CI right now. If you submit a PR then I'll review it but I don't have the time right now to add it myself.
There are a lot of other warnings shown by
staticcheck
that were not fixed (staticcheck
isn't in use for this repo). Most of these warnings are for the checks listed below. Fixing these is a rather big change, just due to volume of lines changed. Whether or not we want to implementstaticcheck
is the question.Changing field names per warning ST1003 is a breaking change, so is probably best to be avoided (staticcheck.conf would be set to
checks="all", "-ST1003"]
). Fixes for the other warnings are no issue since these are just a documentation change.Examples to be fixed...