[Issue 188] Add golangci-lint to subo

[javorszky]

Closes #188

[javorszky]

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

• this handlerData is not being used anywhere, and it's not exported. Normally I would remove it, but seeing that it looks important, I want to hold off on it
• this resourceFlag is not being referenced anywhere either
• nor is this updateFlag

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

[javorszky]

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: 198ad21

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

[javorszky]

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

• updateFlag: feat(cli): Reimplement updateflag #191
• resourceFlag: feat(builder): reimplement resourceFlag #192
• handlerData: feat(builder): reimplement handlerData #193

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 d664574 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]

@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):

subo/.github/workflows/sanity.yml

Lines 29 to 37 in 9249bb7

	- name: Create project and runnable
	run: |
	export PATH=$PATH:$(go env GOPATH)/bin
	git config --global init.defaultBranch main
	subo create project smoketest
	subo create runnable rs-test --lang rust --dir ./smoketest
	subo create runnable swift-test --lang swift --dir ./smoketest
	subo create runnable as-test --lang typescript --dir ./smoketest
	subo create runnable tinygo-test --lang tinygo --dir ./smoketest

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]

@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]

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

[denopink]

@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]

@denopink awesome. I've opened #194 to track readding them with tests.

[denopink]

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]

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

[danielledeleo]

Looks reasonable!

[danielledeleo]

Approved