API improvement : Drain response Body inside func HasScopes - #1428

I really don't think we need such verbosity to simply drain a body. How about:

io.Copy(ioutil.Discard, res.Body)

@mislav Thank you for reviewing the PR.
I completely agree with your point.
In my first commit, I did the same and you could see here. But the golint tool was giving the following error in the CI.

GitHub Actions
/ lint

api/client.go#L221
[errcheck] Error return value of `io.CopyN` is not checked

That's the only reason I added err checks in the second commit so that the job becomes error-free. :)

We can ignore the error but I found that the whole practice (of ignoring errors) is strongly discouraged. Here's what official Effective Go has to say about this:

Occasionally you'll see code that discards the error value in order to ignore the error; this is terrible practice. Always check error returns; they're provided for a reason.

Thanks for explaining! My comment was mostly about maxBodySlurpSize and CopyN; I did not think specifying all that level of detail gets us anything over io.Copy(ioutil.Discard, res.Body).

I get it regarding error handling in Go, but we literally don't care about the response body in this method. Therefore, we also don't care about errors related to res.Body. We only want to close the response so that the connection can get re-used.

Since we don't consume the response body at all, I'm just noticing that we don't need a defer either. Therefore, the whole thing could be replaced with:

_ = io.Copy(ioutil.Discard, res.Body)
_ = res.Body.Close()

Would you agree?

@mislav Thank you.
Yes, I agree with you point that maxBodySlurpSize and CopyN are not actually needed here.

But we need defer here to perform the cleanup task. The reason is it's not always guaranteed that the code may reach the end of the function (e.g. an error or some other condition may force you to return well ahead of the end of a function). The defer statement makes sure that whatever function is assigned to it gets executed for sure even if the function panics or the code returns well before the end of the function.

_ = io.Copy(ioutil.Discard, res.Body) We cannot use single wildcard variable as the function returns two values.

Therefore, keeping your suggestion on the mind, I would suggest:

defer func() {
		_, _  = io.Copy(ioutil.Discard, res.Body)
		res.Body.Close()
	}()

Could you please share your thoughts regarding it? Thanks!

I tried the above changes and all tests have passed successfully. :) 2167689
https://github.com/cli/cli/pull/1428/checks?check_run_id=917723120

But we need defer here to perform the cleanup task. The reason is it's not always guaranteed that the code may reach the end of the function (e.g. an error or some other condition may force you to return well ahead of the end of a function).

Sure, I understand why defer is useful in general! However, there is one more way of ensuring that some code is guaranteed to run: by doing it immediately without defer 😉 There is no reason I can think of that we need to defer the block of code in question, since we can drain and close the body immediately when not consuming it.

However, since it does not make a huge difference either way, I appreciate you fixing the original problem and I will merge as-is. 🙇