Feature: Add logging of HTTP status code

[tisba]

This PR adds logging of the HTTP status code.

I'm not sure if this is the "right" approach, as I'm still pretty new to Golang. I'd appreciate any guidance on how this might be build in a better or more idiomatic way 🙏

Fixes #6

[tisba]

Not sure why, but I get no errors when running golangci-lint run on master locally ¯_(ツ)_/¯

Anyway, the locations that CI reports, AFAIK have nothing to do with my change set.

[syntaqx]

Yeah, this is something that's been popping up in the newest version of Go, it's actually super annoying. I haven't quite nailed down what it is. Just take the variables it's talking about and name them _ and it'll fix the issue

[syntaqx]

Let me actually just make that in a separate PR, one moment.

[syntaqx]

My only main feedback on this, is that I would have probably put statusWriter into a different file, if not a different package entirely. That helps inform others to test it more generally as a standalone object rather than a side-effect of the logger. This pattern is most certainly reusable for other things (ie, # of bytes written, etc). None of this is blocking however, so let's get you merged in.

[syntaqx]

Oop, I didn't realize the codecoverage was lowered based on my previous comment. Can you please ensure we have 100% coverage so I can merge it in?

Seems like you need to add a test where the usecase doesn't set a status code so your default condition kicks in. A table driven test would be great for this in your current implementation.

[tisba]

Ups, I already pushed a change to delint serve_test.go. If you like, you can cherry pick.

[tisba]

Oop, I didn't realize the codecoverage was lowered based on my previous comment. Can you please ensure we have 100% coverage so I can merge it in?

Seems like you need to add a test where the usecase doesn't set a status code so your default condition kicks in. A table driven test would be great for this in your current implementation.

Sure thing! I'll give it a try :)

[tisba]

Okay, so I extracted statusWriter into its own file and also added one more test case without setting the status code (by using table test).

[tisba]

Out of curiosity: You've uploaded a new release to GitHub, but did not update the homebrew-tap repo. Not sure if this was intentional, or an issue; I just wanted to let you know.

[syntaqx]

Hmmm... might have been a bug with the release. Let me dig into it. That def is supposed to be automated.

[syntaqx]

Hopefully should be fixed now

[tisba]

Yes, it does! 👍