From c5440b693481a2157cff749754240531ec964384 Mon Sep 17 00:00:00 2001 From: Cam Hutchison Date: Tue, 22 Sep 2020 17:26:10 +1000 Subject: [PATCH 1/3] Tidy up the Makefile a little Use `$(O)` consistently instead of `$O`. We do that in all our other foxygoat Makefiles. Fix `COVERFILE` to use `$(O)` instead of the old hardcoded `out`. Remove `showcover` from `.PHONY` - this was missed when `showcover` was renamed back to `cover`. --- Makefile | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Makefile b/Makefile index 2220d38..0bf88c3 100644 --- a/Makefile +++ b/Makefile @@ -6,7 +6,7 @@ all: build test check-coverage lint ## build, test, check coverage and lint @echo '$(COLOUR_GREEN)Success$(COLOUR_NORMAL)' clean:: ## Remove generated files - -rm -rf $O + -rm -rf $(O) .PHONY: all clean @@ -14,8 +14,8 @@ clean:: ## Remove generated files # Build all subdirs of ./cmd, excluding those with a leading underscore. CMDDIRS = $(filter-out ./cmd/_%,$(wildcard ./cmd/*)) -build: | $O ## Build binaries of directories in ./cmd to out/ - go build -o $O $(CMDDIRS) +build: | $(O) ## Build binaries of directories in ./cmd to out/ + go build -o $(O) $(CMDDIRS) install: ## Build and install binaries in $GOBIN or $GOPATH/bin go install $(CMDDIRS) @@ -23,10 +23,10 @@ install: ## Build and install binaries in $GOBIN or $GOPATH/bin .PHONY: build install # --- Test --------------------------------------------------------------------- -COVERFILE = out/coverage.txt +COVERFILE = $(O)/coverage.txt COVERAGE = 100 -test: build | $O ## Run tests and generate a coverage file +test: build | $(O) ## Run tests and generate a coverage file go test -coverprofile=$(COVERFILE) ./... ./cmd/timeout/test.sh @@ -39,7 +39,7 @@ cover: test ## Show test coverage in your browser CHECK_COVERAGE = awk -F '[ \t%]+' '/^total:/ {print; if ($$3 < $(COVERAGE)) exit 1}' FAIL_COVERAGE = { echo '$(COLOUR_RED)FAIL - Coverage below $(COVERAGE)%$(COLOUR_NORMAL)'; exit 1; } -.PHONY: check-coverage cover showcover test +.PHONY: check-coverage cover test # --- Lint --------------------------------------------------------------------- GOLINT_VERSION = 1.30.0 @@ -73,7 +73,7 @@ COLOUR_WHITE = $(shell tput setaf 7 2>/dev/null) help: @awk -F ':.*## ' 'NF == 2 && $$1 ~ /^[A-Za-z0-9_-]+$$/ { printf "$(COLOUR_WHITE)%-30s$(COLOUR_NORMAL)%s\n", $$1, $$2}' $(MAKEFILE_LIST) | sort -$O: +$(O): @mkdir -p $@ .PHONY: help From 1962983ed015e520f384e1588f5c8334e2448750 Mon Sep 17 00:00:00 2001 From: Cam Hutchison Date: Tue, 22 Sep 2020 17:59:42 +1000 Subject: [PATCH 2/3] httpe: Tidy up godoc comments Clean up some wording to make the godoc clearer. Add some missing bits that should have been added when the package evolved. --- httpe/err.go | 19 ++++++++++++++----- httpe/httpe.go | 23 ++++++++++++----------- httpe/method.go | 35 ++++++++++++++++++++++++++--------- 3 files changed, 52 insertions(+), 25 deletions(-) diff --git a/httpe/err.go b/httpe/err.go index a779c55..f0b695a 100644 --- a/httpe/err.go +++ b/httpe/err.go @@ -13,8 +13,7 @@ type StatusError int // Error returns the error message and implements the error interface. func (err StatusError) Error() string { return http.StatusText(int(err)) } -// IsClientError returns true if the error is in 4xx range. Useful for -// error message printing and hiding details. +// IsClientError returns true if the error is in range 400 to 499. func (err StatusError) IsClientError() bool { return int(err) >= 400 && int(err) <= 499 } // Code returns a status int representing an http.Status* value. @@ -66,9 +65,19 @@ var ( ErrNetworkAuthenticationRequired = StatusError(http.StatusNetworkAuthenticationRequired) ) -// WriteSafeErr writes the http status represented by err to the ResponseWriter and -// prints the error message for 4xx errors, but keeps details for 5xx errors. -// It writes 5xx errors "safely" by it doesn't leak any error details. +// WriteSafeErr writes err as an HTTP error to the http.ResponseWriter. +// +// If err wraps a StatusError, WriteSafeErr writes the Code of that error as +// the HTTP status code. Otherwise writes 500 as the code (internal server +// error). +// +// WriteSafeErr writes the HTTP response body as the error described as a +// string if the error wraps a StatusError and the Code for that error is a +// client error (code 400 to 499). Otherwise, only the text for the status code +// is written. Errors that do not wrap a StatusError are treated as +// ErrInternalServerError. This prevents leaking internal details from a server +// error into the response to the client, but allows adding information to a +// client error to inform the client of details of that error. func WriteSafeErr(w http.ResponseWriter, err error) { if err == nil { return diff --git a/httpe/httpe.go b/httpe/httpe.go index de4962e..a964f5e 100644 --- a/httpe/httpe.go +++ b/httpe/httpe.go @@ -1,9 +1,10 @@ // Package httpe provides a model of HTTP handler that returns errors. // -// net/http provides a Handler interface that requires the handler to write -// errors to the provided ResponseWriter. This is different to the usual Go way -// of handling errors that has functions returning errors, and it makes normal -// http.Handlers a bit cumbersome. +// The standard Go library package net/http provides a Handler interface that +// requires the handler write errors to the provided ResponseWriter. This is +// different to the usual Go way of handling errors that has functions +// returning errors, and it makes normal http.Handlers a bit cumbersome and +// repetitive in the error handling cases. // // This package provides a HandlerE interface with a ServeHTTPe method that has // the same signature as http.Handler.ServeHTTP except it also returns an @@ -62,15 +63,15 @@ func (ew ErrWriterFunc) WriteErr(w http.ResponseWriter, err error) { ew(w, err) } -// New returns a http.Handler that calls in sequence all the args that are a -// type of handler stopping if any return an error. If any of the args is an +// New returns an http.Handler that calls in sequence all the args that are a +// type of handler, stopping if any return an error. If any of the args is an // ErrWriter or a function that has the signature of an ErrWriterFunc, it will // be called to handle the error if there was one. // // The types that are recognised as handlers in the arg list are any type that // implements HandlerE (including HandlerFuncE), a function that matches the -// signature of a HandlerFuncE, a http.Handler or a function that matches the -// signature of a http.HandlerFunc. Args of the latter two are adapted to +// signature of a HandlerFuncE, an http.Handler, or a function that matches the +// signature of an http.HandlerFunc. Args of the latter two are adapted to // always return a nil error. // // If an argument does not match any of the preceding types or more than one @@ -176,9 +177,9 @@ func WithErrWriterFunc(f ErrWriterFunc) option { //nolint:golint // Do not want } } -// Chain returns a HandlerE which executes each of the HandlerFuncE -// parameters sequentially stopping at the first one that returns an -// error, and returning that error, or nil if none return an error. +// Chain returns a HandlerE that executes each of the HandlerFuncE parameters +// sequentially, stopping at the first one that returns an error and returning +// that error. It returns nil if none of the handlers return an error. func Chain(he ...HandlerE) HandlerE { f := func(w http.ResponseWriter, r *http.Request) error { for _, h := range he { diff --git a/httpe/method.go b/httpe/method.go index 685d139..81ceeed 100644 --- a/httpe/method.go +++ b/httpe/method.go @@ -3,23 +3,40 @@ package httpe import "net/http" var ( - // Get returns a ErrMethodNotAllowed if request method is not a GET. Use with Chain. + // Get is a HandlerE that returns a ErrMethodNotAllowed if the request + // method is not GET. Use with Chain or New/Must. Get = newMethodChecker(http.MethodGet) - // Head returns a ErrMethodNotAllowed if request method is not a HEAD. Use with Chain. + + // Head is a HandlerE that returns a ErrMethodNotAllowed if the request + // method is not HEAD. Use with Chain or New/Must. Head = newMethodChecker(http.MethodHead) - // Post returns a ErrMethodNotAllowed if request method is not a Post. Use with Chain. + + // Post is a HandlerE that returns a ErrMethodNotAllowed if the request + // method is not POST. Use with Chain or New/Must. Post = newMethodChecker(http.MethodPost) - // Put returns a ErrMethodNotAllowed if request method is not a Put. Use with Chain. + + // Put is a HandlerE that returns a ErrMethodNotAllowed if the request + // method is not PUT. Use with Chain or New/Must. Put = newMethodChecker(http.MethodPut) - // Patch returns a ErrMethodNotAllowed if request method is not a Patch. Use with Chain. + + // Patch is a HandlerE that returns a ErrMethodNotAllowed if the + // request method is not PATCH. Use with Chain or New/Must. Patch = newMethodChecker(http.MethodPatch) - // Delete returns a ErrMethodNotAllowed if request method is not a Delete. Use with Chain. + + // Delete is a HandlerE that returns a ErrMethodNotAllowed if the + // request method is not DELETE. Use with Chain or New/Must. Delete = newMethodChecker(http.MethodDelete) - // Connect returns a ErrMethodNotAllowed if request method is not a Connect. Use with Chain. + + // Connect is a HandlerE that returns a ErrMethodNotAllowed if the + // request method is not CONNECT. Use with Chain or New/Must. Connect = newMethodChecker(http.MethodConnect) - // Options returns a ErrMethodNotAllowed if request method is not a Options. Use with Chain. + + // Options is a HandlerE that returns a ErrMethodNotAllowed if the + // request method is not OPTIONS. Use with Chain or New/Must. Options = newMethodChecker(http.MethodOptions) - // Trace returns a ErrMethodNotAllowed if request method is not a Trace. Use with Chain. + + // Trace is a HandlerE that returns a ErrMethodNotAllowed if the + // request method is not TRACE. Use with Chain or New/Must. Trace = newMethodChecker(http.MethodTrace) ) From 4ea3c522ad148bfc6d137194f6ef01e2b466cbed Mon Sep 17 00:00:00 2001 From: Cam Hutchison Date: Tue, 22 Sep 2020 18:17:13 +1000 Subject: [PATCH 3/3] errs: Tidy up godoc comments Clean up a little bit of wording in the godoc comments to try to make it clearer. --- errs/errs.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/errs/errs.go b/errs/errs.go index 35d1e61..efa27d9 100644 --- a/errs/errs.go +++ b/errs/errs.go @@ -1,8 +1,8 @@ // Package errs provides error wrapping functions that allow an error to -// wrap multiple other errors. The error wrapping in the Go standard -// library allows an error to wrap only one error with only one %w +// wrap multiple errors. The error wrapping in the Go standard library +// errors package allows an error to wrap only one error with one %w // format verb in the format string passed to fmt.Errorf and the -// errors.Unwrap() function that returns only a single error. +// errors.Unwrap() function that returns a single error. // // The error type returned by the functions in this package wraps // multiple errors so that errors.Is() and errors.As() can be used to @@ -10,8 +10,8 @@ // on errors of that type always returns nil as that function cannot // return multiple errors. // -// The error type is not exported so is used through the standard Go -// error interfaces. +// The error type is not exported so can only used through the standard +// Go error interfaces. package errs import (