Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consume returned values implicitly, remove some nolint comments #410

Merged
merged 7 commits into from
Nov 18, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add nolint comment to ignore
Signed-off-by: Lam Tran <[email protected]>
tranngoclam committed Nov 17, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 290befeb607d196290cb1c7b80a826f227f9633b
2 changes: 1 addition & 1 deletion internal/envoy/install.go
Original file line number Diff line number Diff line change
@@ -98,7 +98,7 @@ func untarEnvoy(ctx context.Context, dst string, src version.TarballURL, // dst,
if err != nil {
return err
}
defer func() { _ = res.Body.Close() }()
defer res.Body.Close() //nolint

if res.StatusCode != http.StatusOK {
return fmt.Errorf("received %v status code from %s", res.StatusCode, src)
3 changes: 2 additions & 1 deletion internal/moreos/moreos_func-e_test.go
Original file line number Diff line number Diff line change
@@ -18,6 +18,7 @@ import (
"bufio"
"bytes"
"embed"
"errors"
"os"
"os/exec"
"path/filepath"
@@ -176,7 +177,7 @@ func requireFindProcessError(t *testing.T, proc *os.Process, expectedErr error)
// Wait until the operating system removes or adds the scheduled process.
require.Eventually(t, func() bool {
_, err := process.NewProcess(int32(proc.Pid)) // because os.FindProcess is no-op in Linux!
return err == expectedErr
return errors.Is(err, expectedErr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar... is something wrapping this error now?

}, 100*time.Millisecond, 5*time.Millisecond, "timeout waiting for expected error %v", expectedErr)
}

9 changes: 5 additions & 4 deletions internal/tar/tar.go
Original file line number Diff line number Diff line change
@@ -21,6 +21,7 @@ import (
"compress/gzip"
"crypto/sha256"
"encoding/hex"
"errors"
"fmt"
"hash"
"io"
@@ -82,7 +83,7 @@ func Untar(dst string, src io.Reader) error { // dst, src order like io.Copy
tr := tar.NewReader(zSrc)
for {
header, err := tr.Next()
if err == io.EOF {
if errors.Is(err, io.EOF) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is something wrapping the io.EOF now or are you just expecting it to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, my point is keeping the consistency when checking error at the present and also future.
IMHO, we should use errors.Is for all the cases, what do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for letting me know your input, but I don't think we should call a more expensive function when we don't need it. errors.Is handles unwrapping, and there are two places where you changed to use that here where this isn't needed.

First case you changed was in a unit test on code we wrote. We are testing an expected error. To reduce precision in a test is an anti-goal.. ex I would want to know if our code started wrapping errors. Making the assertion more flimsy allows that test to pass even if someone accidentally wrapped the error. Since we control the code we should know what we are doing and be able to tell if we made a mistake.

The second place you changed the expression was unnecessary for a different reason. The reader api documents the result of EOF as a known behavior. We don't need to expect expect tar to suddenly violate that contract and expect if it did, it would be doing that via wrapping.

Even if func-e isn't particularly concerned about code size, memory size or perf, it is important to know what you are doing. In other libraries, we'd want to avoid bloat and performance loss for no reason. For example, you can see that using errors.Is generates larger code than using the simpler and valid form https://godbolt.org/z/fzabhba45

There are cases of style consistency where there could be subtle differences and choosing one over another is sensible. We totally do that. However, this is a behavior change and the choice adds surprise.. For example, I would suggest the otherwise should be true. If you use errors.Is, there should be a good reason to do that. Trigger the exception the opposite way, use == unless something is special. This may not be the same optimization choice for layered apps or apps that use a lot of unknown deps, but that's the rationale here.

Hope this makes sense. One point that remains is you have added more scope to this PR after a request to reduce scope of it. This causes a day round trip and time to go over pros and cons. Changing the code policy of a project is something better to do when actively developing it vs a "drive by". For example, through fixing other issues, things that are priority to fix, the same time is spent and more value brought through discussions and whatnot. I would suggest to be somewhat empathetic to the project teams when doing style changes and not doing more things on each rev, as we end up in discussions like this which consume both of our time even if there is benefit personally, it starves other change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codefromthecrypt
I totally appreciate these points, also reverted the changes.

break
} else if err != nil {
return err
@@ -152,11 +153,11 @@ func TarGz(dst, src string) error { // dst, src order like io.Copy
if err != nil {
return err
}
defer func() { _ = file.Close() }()
defer file.Close() //nolint
gzw := gzip.NewWriter(file)
defer func() { _ = gzw.Close() }()
defer gzw.Close() //nolint
tw := tar.NewWriter(gzw)
defer func() { _ = tw.Close() }()
defer tw.Close() //nolint

// Recurse through the path including all files and directories
return fs.WalkDir(srcFS, basePath, func(path string, d os.DirEntry, err error) error {
4 changes: 2 additions & 2 deletions internal/test/fakebinary/testdata/fake_envoy.go
Original file line number Diff line number Diff line change
@@ -49,7 +49,7 @@ func main() {
// Start a fake admin listener that write the same sort of response Envoy's /ready would, but on all endpoints.
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Envoy console messages all write to stderr. Simulate access_log_path: '/dev/stdout'
_, _ = os.Stdout.Write([]byte(fmt.Sprintf("GET %s HTTP/1.1%s", r.RequestURI, lf)))
os.Stdout.Write([]byte(fmt.Sprintf("GET %s HTTP/1.1%s", r.RequestURI, lf)))

w.Header().Add("Content-Type", "text/plain; charset=UTF-8")
w.WriteHeader(200)
@@ -65,7 +65,7 @@ func main() {
}

// Echo the same line Envoy would on successful startup
_, _ = os.Stderr.Write([]byte("starting main dispatch loop" + lf))
os.Stderr.Write([]byte("starting main dispatch loop" + lf))

// Block until we receive a signal
msg := "unexpected"
2 changes: 1 addition & 1 deletion internal/test/server.go
Original file line number Diff line number Diff line change
@@ -127,7 +127,7 @@ func RequireFakeEnvoyTarGz(t *testing.T, v version.PatchVersion) ([]byte, versio
// Read the tar.gz into a byte array. This allows the mock server to set content length correctly
f, err := os.Open(tempGz)
require.NoError(t, err)
t.Cleanup(func() { require.NoError(t, f.Close()) })
defer f.Close() //nolint
b, err := io.ReadAll(f)
require.NoError(t, err)
return b, version.SHA256Sum(fmt.Sprintf("%x", sha256.Sum256(b)))
2 changes: 1 addition & 1 deletion main_test.go
Original file line number Diff line number Diff line change
@@ -29,7 +29,7 @@ func TestRunErrors(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(200)
}))
t.Cleanup(func() { server.Close() })
t.Cleanup(server.Close)

tests := []struct {
name string