[FIX] Cleanup code #1061

im-kulikov · 2018-02-10T16:18:55Z

cleanup
fix collides with package name
optimize slice creation
fix variable name shadowing

- cleanup - fix `collides with package name` - optimize slice creation - fix variable name shadowing

codecov · 2018-02-10T16:22:49Z

Codecov Report

Merging #1061 into master will increase coverage by 1.46%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1061      +/-   ##
==========================================
+ Coverage      79%   80.46%   +1.46%     
==========================================
  Files          27       27              
  Lines        1881     1879       -2     
==========================================
+ Hits         1486     1512      +26     
+ Misses        281      259      -22     
+ Partials      114      108       -6

Impacted Files	Coverage Δ
middleware/body_dump.go	`82.85% <ø> (+22.85%)`	⬆️
middleware/logger.go	`84.46% <ø> (ø)`	⬆️
middleware/proxy.go	`64.35% <0%> (+15.84%)`	⬆️
middleware/jwt.go	`88.57% <0%> (ø)`	⬆️
middleware/csrf.go	`76.71% <100%> (ø)`	⬆️
bind.go	`83.22% <100%> (ø)`	⬆️
echo.go	`86.38% <100%> (ø)`	⬆️
router.go	`93.2% <100%> (ø)`	⬆️
middleware/basic_auth.go	`68.57% <100%> (+9.11%)`	⬆️
group.go	`93.75% <100%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7eec915...626e513. Read the comment docs.

im-kulikov · 2018-02-11T09:19:28Z

@vishr any updates?

im-kulikov · 2018-02-12T16:41:53Z

@vishr any updates before merge?

im-kulikov · 2018-02-20T12:52:32Z

@vishr any updates?

vishr · 2018-02-20T16:31:55Z

bind_test.go

@@ -135,7 +135,7 @@ func TestBindForm(t *testing.T) {
 	rec := httptest.NewRecorder()
 	c := e.NewContext(req, rec)
 	req.Header.Set(HeaderContentType, MIMEApplicationForm)
-	obj := []struct{ Field string }{}
+	var obj []struct{ Field string }


We prefer without var across the project unless it is really needed, so lets stick to it.

@vishr fixed

+1 for short variable declaration form (:=) in general, but maybe with the exception for empty slices? Just like https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices recommends.

While we're at it, maybe we should add a note about the coding style to use to the "Contributing" section of the README?

I'll fix it with (just more simple):

err := c.Bind(&[]struct{ Field string }{})

without variable declaration

vishr · 2018-02-20T16:33:22Z

echo.go

@@ -506,7 +506,7 @@ func (e *Echo) URL(h HandlerFunc, params ...interface{}) string {

 // Reverse generates an URL from route name and provided parameters.
 func (e *Echo) Reverse(name string, params ...interface{}) string {
-	uri := new(bytes.Buffer)
+	uri := bytes.NewBuffer(nil)


For consistency, please search and replace similar statements across the project.

I think var uri bytes.Buffer is more reasonable.

var uri bytes.Buffer not equal uri := bytes.NewBuffer(nil)

vishr · 2018-02-20T16:35:58Z

echo.go

@@ -530,7 +530,7 @@ func (e *Echo) Reverse(name string, params ...interface{}) string {

 // Routes returns the registered routes.
 func (e *Echo) Routes() []*Route {
-	routes := []*Route{}


This statement should be fine as we are reassigning the variable.

routes := make([]*Route, 0, len(e.router.routes))

this is prealloc space for this part:

for _, v := range e.router.routes { routes = append(routes, v) }

If you preallocate it then would it be simpler to do this instead:

routes := make([]*Route, len(e.router.routes)) for i, v := range e.router.routes { routes[i] = v }

You can avoid the append call.

@alexaandru e.router.routes is map[string]*Route
that's why i is a string, not an integer

I stand corrected @im-kulikov

Looked into append function so it uses the preallocated capacity, so we are good 👍

vishr · 2018-02-20T16:37:16Z

group.go

@@ -92,7 +92,7 @@ func (g *Group) Match(methods []string, path string, handler HandlerFunc, middle

 // Group creates a new sub-group with prefix and optional sub-group-level middleware.
 func (g *Group) Group(prefix string, middleware ...MiddlewareFunc) *Group {
-	m := []MiddlewareFunc{}


Same as above

vishr · 2018-02-20T16:37:30Z

group.go

@@ -113,7 +113,7 @@ func (g *Group) Add(method, path string, handler HandlerFunc, middleware ...Midd
 	// Combine into a new slice to avoid accidentally passing the same slice for
 	// multiple routes, which would lead to later add() calls overwriting the
 	// middleware from earlier calls.
-	m := []MiddlewareFunc{}
+	var m []MiddlewareFunc


https://play.golang.org/p/1TqVZ4GVhnF

vishr · 2018-02-20T16:37:40Z

middleware/basic_auth.go

@@ -93,7 +93,7 @@ func BasicAuthWithConfig(config BasicAuthConfig) echo.MiddlewareFunc {
 				}
 			}

-			realm := ""
+			var realm string


ineffectual assignment

How about this?

realm := defaultRealm if config.Realm != defaultRealm { realm = strconv.Quote(config.Realm) }

or maybe have defaultRealm unquoted and the entire variable and if can go away, and on the response we can simply use strconv.Quote(config.Realm) directly.

thanks.. fixed

vishr · 2018-02-20T16:43:14Z

echo.go

+	ErrValidatorNotRegistered      = errors.New("validator not registered")
+	ErrRendererNotRegistered       = errors.New("renderer not registered")
+	ErrInvalidRedirectCode         = errors.New("invalid redirect status code")
+	ErrCookieNotFound              = errors.New("cookie not found")


We keep all errors to lower case as it makes it easy to append further.

Error strings should not start with a capital letter (https://github.com/golang/go/wiki/Errors)

I meant the same, please search and replace across the project.

👌 no problem

vishr · 2018-02-20T16:50:35Z

router.go

@@ -59,8 +59,8 @@ func (r *Router) Add(method, path string, h HandlerFunc) {
 	if path[0] != '/' {
 		path = "/" + path
 	}
-	ppath := path        // Pristine path


Same as above and let's revert all the renaming for consistency.

vishr · 2018-02-20T16:51:06Z

middleware/proxy.go

@@ -108,15 +108,15 @@ func proxyRaw(t *ProxyTarget, c echo.Context) http.Handler {
 			return
 		}

-		errc := make(chan error, 2)
+		errChan := make(chan error, 2)


Please revert the naming to original for consistency.

isn't it more readable and clear?

Ok, let's make it errCh.

vishr · 2018-02-20T16:51:21Z

middleware/logger.go

-						cookie, err := c.Cookie(tag[7:])
-						if err == nil {
+						cookie, errCookie := c.Cookie(tag[7:])
+						if errCookie == nil {


Let's revert to original names.

err variable shadowing

vishr · 2018-02-20T16:51:35Z

middleware/jwt.go

@@ -116,7 +116,7 @@ func JWTWithConfig(config JWTConfig) echo.MiddlewareFunc {
 	config.keyFunc = func(t *jwt.Token) (interface{}, error) {
 		// Check the signing method
 		if t.Method.Alg() != config.SigningMethod {
-			return nil, fmt.Errorf("Unexpected jwt signing method=%v", t.Header["alg"])
+			return nil, fmt.Errorf("unexpected jwt signing method=%v", t.Header["alg"])


Keep errors to lowercase.

You are ok.

vishr · 2018-02-20T18:54:17Z

@im-kulikov I am ok to replace all := short assignment to var for empty initialization. Please make sure we are consistent across the project.

im-kulikov · 2018-02-20T20:24:52Z

@vishr ok. thanks for explain

- errors in lowercase - bytes.NewBuffer() - `errCookie` -> `err` (hmm.. `err` variable shadowing) - `errChan` -> `errCh`

im-kulikov · 2018-02-21T08:28:17Z

@vishr hi! can you check now..

alexaandru · 2018-02-21T08:41:46Z

router.go

@@ -59,8 +59,8 @@ func (r *Router) Add(method, path string, h HandlerFunc) {
 	if path[0] != '/' {
 		path = "/" + path
 	}
-	ppath := path        // Pristine path
-	pnames := []string{} // Param names
+	pPath := path       // Pristine path


I'm all for short variable names, but if we need to comment variable names, maybe we should at least consider using longer names? These names seems to be used away enough from where they were declared that they qualify for longer names. paramNames, etc.

this is internal name.. and has comment

We stick to short names as Go guidelines recommend - specially used internally.

I'll revert this part

im-kulikov · 2018-02-21T17:57:06Z

@vishr there is any updates?

vishr · 2018-02-21T18:10:00Z

@im-kulikov I gave a second thought later and have decided as below:

Between := vs var and I would like to stick with := as much as possible.
For bytes initialization let's revert to new(bytes.Buffer), for reference https://golang.org/pkg/bytes/#NewBuffer
Let's keep the internal names as is for now e.g. pnames, pvalues, err

vishr · 2018-02-21T18:24:09Z

echo.go

-		path := r.URL.RawPath
-		if path == "" {
-			path = r.URL.Path
+		rpath := r.URL.RawPath // raw path


rawPath is fine here

path collides with imported package name.. that's why I use rpath with a comment

vishr · 2018-02-21T18:26:43Z

router.go

-	ppath := path        // Pristine path
-	pnames := []string{} // Param names
+	ppath := path       // Pristine path
+	var pNames []string // Param names


Please revert this and rename to pnames.

vishr · 2018-02-21T18:27:25Z

middleware/proxy.go

 		cp := func(dst io.Writer, src io.Reader) {
-			_, err := io.Copy(dst, src)
-			errc <- err
+			_, errCopy := io.Copy(dst, src)


can you reuse err?

@alexaandru

@alexaandru if you just don't know how it works, please.. don't give advice ``` panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x12e9b34] goroutine 50 [running]: testing.tRunner.func1(0xc420518000) /usr/local/Cellar/go/1.10/libexec/src/testing/testing.go:742 +0x29d panic(0x1349aa0, 0x1593f70) /usr/local/Cellar/go/1.10/libexec/src/runtime/panic.go:505 +0x229 github.com/labstack/echo/middleware.CSRFWithConfig.func1.1(0x13ff9a0, 0xc4205005b0, 0x13babed, 0x13) /Users/kulikov/.golang/src/github.com/labstack/echo/middleware/csrf.go:129 +0xe4 github.com/labstack/echo/middleware.TestCSRF(0xc420518000) /Users/kulikov/.golang/src/github.com/labstack/echo/middleware/csrf_test.go:28 +0x2da testing.tRunner(0xc420518000, 0x13ce290) /usr/local/Cellar/go/1.10/libexec/src/testing/testing.go:777 +0xd0 created by testing.(*T).Run /usr/local/Cellar/go/1.10/libexec/src/testing/testing.go:824 +0x2e0 FAIL github.com/labstack/echo/middleware 0.119s ```

im-kulikov · 2018-02-21T18:35:44Z

@vishr fixed, something else?

vishr · 2018-02-21T18:42:46Z

@im-kulikov thanks for your contribution, I really appreciated it. On a side note, and lets be polite in the conversation @alexaandru is new to the project and Go (I believe), everybody here is to learn, collaborate and have fun!!! 🎉

im-kulikov · 2018-02-21T18:43:51Z

em.. just closed?

vishr · 2018-02-21T18:44:41Z

@im-kulikov just a mistake - don't worry 😉

im-kulikov · 2018-02-21T18:45:39Z

@vishr @alexaandru Oh.. I'm sorry if I hurt or hooked you. I was referring to the fact that it is not necessary to give advice without verifying assumptions.. I had to go crazy and look for a problem.

alexaandru · 2018-02-21T18:56:08Z

I'm all good @im-kulikov @vishr haven't seen anything even remotely unpolite :) and I'm sorry if any of my advices caused troubles. May I know which one that was? Cheers!

im-kulikov · 2018-02-21T18:58:11Z

@alexaandru this one - #1061 (comment)

vishr · 2018-02-21T19:02:18Z

@im-kulikov @alexaandru can you guys join gitter chat? @im-kulikov If you are interested in fixing autocert please let me know labstack/armor#30

im-kulikov · 2018-02-21T19:03:19Z

@vishr if it needed :)

Enhancements: Implemented Response#After() Dynamically add/remove proxy targets Rewrite rules for proxy middleware Add ability to extract key from a form field Implemented rewrite middleware Adds a separate flag for the 'http/https server started on' message (#1043) Applied a little DRY to the redirect middleware (#1053) and tests (#1055) Simplify dep fetching (#1062) Add custom time stamp format #1046 (#1066) Update property name & default value & comment about custom logger Add X-Requested-With header constant Return error of context.File in c.contentDisposition Updated deps Updated README.md Fixes: Fixed Response#Before() Fixed #990 Fix href url from armor to echo (#1051) Fixed #1054 Fixed #1052, dropped param alias feature Avoid redirect loop in HTTPSRedirect middleware (#1058) Fix deferring body close in middleware/compress test (#1063) Cleanup code (#1061) FIX - We must close gzip.Reader, only if no error (#1069) Fix formatting (#1071) Can be a fix for auto TLS

[FIX] Cleanup code

e2f721c

- cleanup - fix `collides with package name` - optimize slice creation - fix variable name shadowing

vishr reviewed Feb 20, 2018

View reviewed changes

revert

cd1b4b4

vishr reviewed Feb 20, 2018

View reviewed changes

vishr assigned im-kulikov Feb 20, 2018

im-kulikov and others added 6 commits February 21, 2018 10:57

review comments fixes:

8bbde9d

- errors in lowercase - bytes.NewBuffer() - `errCookie` -> `err` (hmm.. `err` variable shadowing) - `errChan` -> `errCh`

Merge branch 'master' into cleanup-code

a5ce536

- revert pPath -> ppath

97daec8

- gofmt / goimports

978e3c3

- simplify

70f88ce

- prealloc

1303ad0

alexaandru reviewed Feb 21, 2018

View reviewed changes

im-kulikov added 4 commits February 21, 2018 11:48

- defaultRealm + tests (thanks @alexaandru)

317e294

add tests for NewRandomBalancer AddTarget / RemoveTarget

5bf9b1d

add tests for NewRandomBalancer AddTarget / RemoveTarget

5648c86

body_dump add tests

88ccfa2

im-kulikov added 4 commits February 21, 2018 21:13

revert bytes.NewBuffer(nil) -> new(bytes.Buffer)

af56088

revert rawPath -> rpath (path collides with imported package name)

9615db0

revert var reqBody []byte -> reqBody := []byte{}

f28a90b

simplify var token string

7fdcece

vishr reviewed Feb 21, 2018

View reviewed changes

im-kulikov added 2 commits February 21, 2018 21:29

revert pNames -> pnames

519812b

vishr closed this Feb 21, 2018

vishr reopened this Feb 21, 2018

vishr merged commit f49d166 into labstack:master Feb 21, 2018

im-kulikov deleted the cleanup-code branch February 21, 2018 18:45

[FIX] Cleanup code #1061

[FIX] Cleanup code #1061

Conversation

im-kulikov commented Feb 10, 2018

codecov bot commented Feb 10, 2018 • edited Loading

Codecov Report

im-kulikov commented Feb 11, 2018

im-kulikov commented Feb 12, 2018

im-kulikov commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-kulikov Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-kulikov Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishr commented Feb 20, 2018

im-kulikov commented Feb 20, 2018

im-kulikov commented Feb 21, 2018

alexaandru Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-kulikov commented Feb 21, 2018

vishr commented Feb 21, 2018

vishr Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-kulikov commented Feb 21, 2018

vishr commented Feb 21, 2018

im-kulikov commented Feb 21, 2018

vishr commented Feb 21, 2018

im-kulikov commented Feb 21, 2018

alexaandru commented Feb 21, 2018

im-kulikov commented Feb 21, 2018

vishr commented Feb 21, 2018 • edited Loading

im-kulikov commented Feb 21, 2018

codecov bot commented Feb 10, 2018 •

edited

Loading

im-kulikov Feb 21, 2018 •

edited

Loading

im-kulikov Feb 21, 2018 •

edited

Loading

alexaandru Feb 21, 2018 •

edited

Loading

vishr Feb 21, 2018 •

edited

Loading

vishr commented Feb 21, 2018 •

edited

Loading