search

suborbital / subo

The Suborbital CLI
Apache License 2.0
81 stars 24 forks source link

[Issue 188] Add golangci-lint to subo #189

Closed javorszky closed 2 years ago

javorszky commented 2 years ago

Closes #188

javorszky commented 2 years ago

So this one fails because of deadcode issues in the following three places:

Can y'all provide more context on what they are (supposed to be) used for? 🙂

javorszky commented 2 years ago

Okay, so after a bit more digging

handlerData

There is no mention of anywhere else in the codebase. It is also not exported, so nothing that imports subo could potentially use it anyways.

I recommend removing it.

resourceFlag

This is not exported from the subo/command package, so scn can't use it. The usage of the property is not found in the entirety of my projects folder which has most of the suborbital repos cloned locally. This is truly not being used anywhere.

I recommend removing it.

updateFlag

This is being used in two other places within subo itself: here and here, but both places are commented out.

They were removed in this commit: https://github.com/suborbital/subo/commit/198ad21b40fa56a7fcf90932e215a56b30e77b4f

This is not being used in any of the other projects either. Recommend removing and maybe adding a @todo comment?

javorszky commented 2 years ago

I removed the unused pieces of code and created issues to track them:

The linter unparam also flagged up a bunch of issues in the docs builder. The function getCodeText received both the docTemplate and actionTemplate, but every caller passed in a constant which doesn't change.

Commit https://github.com/suborbital/subo/pull/189/commits/d6645742a4da35539e61c453bc8941d7ef20699f moves them out from being function arguments so the constants themselves can be used as args in whatever functionality needs them within the getCodeText function.

An alternative solution to the unparam, if we want to keep them as function arguments, is to write a test to the getCodeText function and pass in different templates for both and test that the text that comes out is what we expect to happen given the template we passed in. cc @denopink

denopink commented 2 years ago

@javorszky that totally works for me. We do need the params s.t. it could be used for other types of docs (requiring us to swap out the template/action). When you say tests, are we thinking go unit tests? atm subo's tests are written and ran in GitHub (more like integration tests): https://github.com/suborbital/subo/blob/9249bb720ea332e017190606a247d5cf25b5c4ca/.github/workflows/sanity.yml#L29-L37

I can totally take care of the integration test for subo doc, but should we start introducing unit tests starting with getCodeText (to resolve the unparam)?

I'm all for it and I can take care of it, want me to create a separate ticket for that?

javorszky commented 2 years ago

@denopink Ideally we should be unit testing every piece of code as well. I'm a fan of table driven tests, and the stretchify/assert package (this one: https://github.com/stretchr/testify), and mockery (https://github.com/vektra/mockery) if we need to create controllable mocks of interfaces. Let me know if I can help with any of it.

I tried creating a test function for the getCodeText function in the same directory in a file called docs_test.go, but as I'm not entirely familiar with all the pieces that the functions accept and what data looks like, I opted for the easier removal of arguments while still preserving the functionality. If you'd like I can push up a stub test file.

javorszky commented 2 years ago

Also, thank you for the link to the integration tests 🙂

denopink commented 2 years ago

@javorszky I agree with just removing the params for now so it doesn't block this pr (it'll be just as easy to add them later).

javorszky commented 2 years ago

@denopink awesome. I've opened https://github.com/suborbital/subo/issues/194 to track readding them with tests.

denopink commented 2 years ago

I haven't done table driven tests outside of basic tutorials, so I would love to help with that. that includes interface mockeries as well!

javorszky commented 2 years ago

All right, I'll put some documentation / examples together and share it here later today :)