From 5a57eaa743ac8bace699914012c0f766dd6cbe06 Mon Sep 17 00:00:00 2001 From: Jeevanandam M Date: Wed, 8 Jun 2016 19:32:13 -0700 Subject: [PATCH] revel/revel#1057 code improvements --- harness/app.go | 10 +++++-- harness/build.go | 28 ++++++++++++------ harness/harness.go | 33 ++++++++++++++------- harness/reflect.go | 64 ++++++++++++++++++++++++----------------- harness/reflect_test.go | 36 +++++++++++------------ revel/run.go | 4 +-- revel/test.go | 2 +- 7 files changed, 106 insertions(+), 71 deletions(-) diff --git a/harness/app.go b/harness/app.go index 73d33f7..63e0060 100644 --- a/harness/app.go +++ b/harness/app.go @@ -20,11 +20,12 @@ type App struct { cmd AppCmd // The last cmd returned. } +// NewApp returns app instance with binary path in it func NewApp(binPath string) *App { return &App{BinaryPath: binPath} } -// Return a command to run the app server using the current configuration. +// Cmd returns a command to run the app server using the current configuration. func (a *App) Cmd() AppCmd { a.cmd = NewAppCmd(a.BinaryPath, a.Port) return a.cmd @@ -41,6 +42,7 @@ type AppCmd struct { *exec.Cmd } +// NewAppCmd returns the AppCmd with parameters initialized for running app func NewAppCmd(binPath string, port int) AppCmd { cmd := exec.Command(binPath, fmt.Sprintf("-port=%d", port), @@ -70,6 +72,8 @@ func (cmd AppCmd) Start() error { case <-listeningWriter.notifyReady: return nil } + + // TODO remove this unreachable code and document it panic("Impossible") } @@ -81,7 +85,7 @@ func (cmd AppCmd) Run() { } } -// Terminate the app server if it's running. +// Kill terminates the app server if it's running. func (cmd AppCmd) Kill() { if cmd.Cmd != nil && (cmd.ProcessState == nil || !cmd.ProcessState.Exited()) { revel.TRACE.Println("Killing revel server pid", cmd.Process.Pid) @@ -96,7 +100,7 @@ func (cmd AppCmd) Kill() { func (cmd AppCmd) waitChan() <-chan struct{} { ch := make(chan struct{}, 1) go func() { - cmd.Wait() + _ = cmd.Wait() ch <- struct{}{} }() return ch diff --git a/harness/build.go b/harness/build.go index 98f4a9b..4b4d5a1 100755 --- a/harness/build.go +++ b/harness/build.go @@ -44,8 +44,8 @@ func Build(buildFlags ...string) (app *App, compileError *revel.Error) { "ImportPaths": calcImportAliases(sourceInfo), "TestSuites": sourceInfo.TestSuites(), } - genSource("tmp", "main.go", MAIN, templateArgs) - genSource("routes", "routes.go", ROUTES, templateArgs) + genSource("tmp", "main.go", RevelMainTemplate, templateArgs) + genSource("routes", "routes.go", RevelRoutesTemplate, templateArgs) // Read build config. buildTags := revel.Config.StringDefault("build.tags", "") @@ -126,6 +126,8 @@ func Build(buildFlags ...string) (app *App, compileError *revel.Error) { // Success getting the import, attempt to build again. } + + // TODO remove this unreachable code and document it revel.ERROR.Fatalf("Not reachable") return nil, nil } @@ -179,7 +181,10 @@ func cleanDir(dir string) { revel.ERROR.Println("Failed to clean dir:", err) } } else { - defer f.Close() + defer func() { + _ = f.Close() + }() + infos, err := f.Readdir(0) if err != nil { if !os.IsNotExist(err) { @@ -221,12 +226,14 @@ func genSource(dir, filename, templateSource string, args map[string]interface{} // Create the file file, err := os.Create(filepath.Join(tmpPath, filename)) - defer file.Close() if err != nil { revel.ERROR.Fatalf("Failed to create file: %v", err) } - _, err = file.WriteString(sourceCode) - if err != nil { + defer func() { + _ = file.Close() + }() + + if _, err = file.WriteString(sourceCode); err != nil { revel.ERROR.Fatalf("Failed to write to file: %v", err) } } @@ -345,7 +352,8 @@ func newCompileError(output []byte) *revel.Error { return compileError } -const MAIN = `// GENERATED CODE - DO NOT EDIT +// RevelMainTemplate template for app/tmp/main.go +const RevelMainTemplate = `// GENERATED CODE - DO NOT EDIT package main import ( @@ -399,7 +407,9 @@ func main() { revel.Run(*port) } ` -const ROUTES = `// GENERATED CODE - DO NOT EDIT + +// RevelRoutesTemplate template for app/conf/routes +const RevelRoutesTemplate = `// GENERATED CODE - DO NOT EDIT package routes import "github.com/revel/revel" @@ -415,7 +425,7 @@ func (_ t{{$c.StructName}}) {{.Name}}({{range .Args}} args := make(map[string]string) {{range .Args}} revel.Unbind(args, "{{.Name}}", {{.Name}}){{end}} - return revel.MainRouter.Reverse("{{$c.StructName}}.{{.Name}}", args).Url + return revel.MainRouter.Reverse("{{$c.StructName}}.{{.Name}}", args).URL } {{end}} {{end}} diff --git a/harness/harness.go b/harness/harness.go index a6ae1cc..4d46713 100644 --- a/harness/harness.go +++ b/harness/harness.go @@ -84,12 +84,14 @@ func NewHarness() *Harness { // Prefer the app's views/errors directory, and fall back to the stock error pages. revel.MainTemplateLoader = revel.NewTemplateLoader( []string{filepath.Join(revel.RevelPath, "templates")}) - revel.MainTemplateLoader.Refresh() + if err := revel.MainTemplateLoader.Refresh(); err != nil { + revel.ERROR.Println(err) + } - addr := revel.HttpAddr + addr := revel.HTTPAddr port := revel.Config.IntDefault("harness.port", 0) scheme := "http" - if revel.HttpSsl { + if revel.HTTPSsl { scheme = "https" } @@ -110,7 +112,7 @@ func NewHarness() *Harness { proxy: httputil.NewSingleHostReverseProxy(serverURL), } - if revel.HttpSsl { + if revel.HTTPSsl { harness.proxy.Transport = &http.Transport{ TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, } @@ -166,13 +168,16 @@ func (h *Harness) Run() { watcher.Listen(h, paths...) go func() { - addr := fmt.Sprintf("%s:%d", revel.HttpAddr, revel.HttpPort) + addr := fmt.Sprintf("%s:%d", revel.HTTPAddr, revel.HTTPPort) revel.INFO.Printf("Listening on %s", addr) var err error - if revel.HttpSsl { - err = http.ListenAndServeTLS(addr, revel.HttpSslCert, - revel.HttpSslKey, h) + if revel.HTTPSsl { + err = http.ListenAndServeTLS( + addr, + revel.HTTPSslCert, + revel.HTTPSslKey, + h) } else { err = http.ListenAndServe(addr, h) } @@ -213,7 +218,7 @@ func proxyWebsocket(w http.ResponseWriter, r *http.Request, host string) { d net.Conn err error ) - if revel.HttpSsl { + if revel.HTTPSsl { // since this proxy isn't used in production, // it's OK to set InsecureSkipVerify to true // no need to add another configuration option. @@ -236,8 +241,14 @@ func proxyWebsocket(w http.ResponseWriter, r *http.Request, host string) { revel.ERROR.Printf("Hijack error: %v", err) return } - defer nc.Close() - defer d.Close() + defer func() { + if err = nc.Close(); err != nil { + revel.ERROR.Println(err) + } + if err = d.Close(); err != nil { + revel.ERROR.Println(err) + } + }() err = r.Write(d) if err != nil { diff --git a/harness/reflect.go b/harness/reflect.go index d027f5f..853f76c 100644 --- a/harness/reflect.go +++ b/harness/reflect.go @@ -60,12 +60,14 @@ type methodCall struct { Names []string } +// MethodSpec holds the information of one Method type MethodSpec struct { Name string // Name of the method, e.g. "Index" Args []*MethodArg // Argument descriptors RenderCalls []*methodCall // Descriptions of Render() invocations from this Method. } +// MethodArg holds the information of one argument type MethodArg struct { Name string // Name of the argument. TypeExpr TypeExpr // The name of the type, e.g. "int", "*pkg.UserType" @@ -80,8 +82,9 @@ type embeddedTypeName struct { // receiver. type methodMap map[string][]*MethodSpec -// Parse the app controllers directory and return a list of the controller types found. -// Returns a CompileError if the parsing fails. +// ProcessSource parses the app controllers directory and +// returns a list of the controller types found. +// Otherwise CompileError if the parsing fails. func ProcessSource(roots []string) (*SourceInfo, *revel.Error) { var ( srcInfo *SourceInfo @@ -120,7 +123,7 @@ func ProcessSource(roots []string) (*SourceInfo, *revel.Error) { }, 0) if err != nil { if errList, ok := err.(scanner.ErrorList); ok { - var pos token.Position = errList[0].Pos + var pos = errList[0].Pos compileError = &revel.Error{ SourceType: ".go source", Title: "Go Compilation Error", @@ -139,6 +142,8 @@ func ProcessSource(roots []string) (*SourceInfo, *revel.Error) { return compileError } + + // This is exception, err alredy checked above. Here just a print ast.Print(nil, err) log.Fatalf("Failed to parse dir: %s", err) } @@ -490,7 +495,7 @@ func appendAction(fset *token.FileSet, mm methodMap, decl ast.Decl, pkgImportPat }) var recvTypeName string - var recvType ast.Expr = funcDecl.Recv.List[0].Type + var recvType = funcDecl.Recv.List[0].Type if recvStarType, ok := recvType.(*ast.StarExpr); ok { recvTypeName = recvStarType.X.(*ast.Ident).Name } else { @@ -673,6 +678,8 @@ func (s *SourceInfo) TypesThatEmbed(targetType string) (filtered []*TypeInfo) { return } +// ControllerSpecs returns the all the contollers that embeds +// `revel.Controller` func (s *SourceInfo) ControllerSpecs() []*TypeInfo { if s.controllerSpecs == nil { s.controllerSpecs = s.TypesThatEmbed(revel.RevelImportPath + ".Controller") @@ -680,6 +687,8 @@ func (s *SourceInfo) ControllerSpecs() []*TypeInfo { return s.controllerSpecs } +// TestSuites returns the all the Application tests that embeds +// `testing.TestSuite` func (s *SourceInfo) TestSuites() []*TypeInfo { if s.testSuites == nil { s.testSuites = s.TypesThatEmbed(revel.RevelImportPath + "/testing.TestSuite") @@ -705,7 +714,7 @@ func (e TypeExpr) TypeName(pkgOverride string) string { return e.Expr[:e.pkgIndex] + pkgName + "." + e.Expr[e.pkgIndex:] } -// This returns the syntactic expression for referencing this type in Go. +// NewTypeExpr returns the syntactic expression for referencing this type in Go. func NewTypeExpr(pkgName string, expr ast.Expr) TypeExpr { switch t := expr.(type) { case *ast.Ident: @@ -731,31 +740,32 @@ func NewTypeExpr(pkgName string, expr ast.Expr) TypeExpr { return TypeExpr{Valid: false} } -var _BUILTIN_TYPES = map[string]struct{}{ - "bool": struct{}{}, - "byte": struct{}{}, - "complex128": struct{}{}, - "complex64": struct{}{}, - "error": struct{}{}, - "float32": struct{}{}, - "float64": struct{}{}, - "int": struct{}{}, - "int16": struct{}{}, - "int32": struct{}{}, - "int64": struct{}{}, - "int8": struct{}{}, - "rune": struct{}{}, - "string": struct{}{}, - "uint": struct{}{}, - "uint16": struct{}{}, - "uint32": struct{}{}, - "uint64": struct{}{}, - "uint8": struct{}{}, - "uintptr": struct{}{}, +var builtInTypes = map[string]struct{}{ + "bool": {}, + "byte": {}, + "complex128": {}, + "complex64": {}, + "error": {}, + "float32": {}, + "float64": {}, + "int": {}, + "int16": {}, + "int32": {}, + "int64": {}, + "int8": {}, + "rune": {}, + "string": {}, + "uint": {}, + "uint16": {}, + "uint32": {}, + "uint64": {}, + "uint8": {}, + "uintptr": {}, } +// IsBuiltinType checks the given type is built-in types of Go func IsBuiltinType(name string) bool { - _, ok := _BUILTIN_TYPES[name] + _, ok := builtInTypes[name] return ok } diff --git a/harness/reflect_test.go b/harness/reflect_test.go index d348cf7..9815ca2 100644 --- a/harness/reflect_test.go +++ b/harness/reflect_test.go @@ -92,18 +92,18 @@ func TestGetValidationKeys(t *testing.T) { } var TypeExprs = map[string]TypeExpr{ - "int": TypeExpr{"int", "", 0, true}, - "*int": TypeExpr{"*int", "", 1, true}, - "[]int": TypeExpr{"[]int", "", 2, true}, - "...int": TypeExpr{"[]int", "", 2, true}, - "[]*int": TypeExpr{"[]*int", "", 3, true}, - "...*int": TypeExpr{"[]*int", "", 3, true}, - "MyType": TypeExpr{"MyType", "pkg", 0, true}, - "*MyType": TypeExpr{"*MyType", "pkg", 1, true}, - "[]MyType": TypeExpr{"[]MyType", "pkg", 2, true}, - "...MyType": TypeExpr{"[]MyType", "pkg", 2, true}, - "[]*MyType": TypeExpr{"[]*MyType", "pkg", 3, true}, - "...*MyType": TypeExpr{"[]*MyType", "pkg", 3, true}, + "int": {"int", "", 0, true}, + "*int": {"*int", "", 1, true}, + "[]int": {"[]int", "", 2, true}, + "...int": {"[]int", "", 2, true}, + "[]*int": {"[]*int", "", 3, true}, + "...*int": {"[]*int", "", 3, true}, + "MyType": {"MyType", "pkg", 0, true}, + "*MyType": {"*MyType", "pkg", 1, true}, + "[]MyType": {"[]MyType", "pkg", 2, true}, + "...MyType": {"[]MyType", "pkg", 2, true}, + "[]*MyType": {"[]*MyType", "pkg", 3, true}, + "...*MyType": {"[]*MyType", "pkg", 3, true}, } func TestTypeExpr(t *testing.T) { @@ -126,10 +126,10 @@ func TestTypeExpr(t *testing.T) { } if array { - expr = &ast.ArrayType{expr.Pos(), nil, expr} + expr = &ast.ArrayType{Lbrack: expr.Pos(), Len: nil, Elt: expr} } if ellipsis { - expr = &ast.Ellipsis{expr.Pos(), expr} + expr = &ast.Ellipsis{Ellipsis: expr.Pos(), Elt: expr} } actual := NewTypeExpr("pkg", expr) @@ -146,11 +146,11 @@ func TestProcessBookingSource(t *testing.T) { t.Fatal("Failed to process booking source with error:", err) } - CONTROLLER_PKG := "github.com/revel/samples/booking/app/controllers" + controllerPackage := "github.com/revel/samples/booking/app/controllers" expectedControllerSpecs := []*TypeInfo{ - {"GorpController", CONTROLLER_PKG, "controllers", nil, nil}, - {"Application", CONTROLLER_PKG, "controllers", nil, nil}, - {"Hotels", CONTROLLER_PKG, "controllers", nil, nil}, + {"GorpController", controllerPackage, "controllers", nil, nil}, + {"Application", controllerPackage, "controllers", nil, nil}, + {"Hotels", controllerPackage, "controllers", nil, nil}, } if len(sourceInfo.ControllerSpecs()) != len(expectedControllerSpecs) { t.Errorf("Unexpected number of controllers found. Expected %d, Found %d", diff --git a/revel/run.go b/revel/run.go index 0c4841c..4d4572f 100644 --- a/revel/run.go +++ b/revel/run.go @@ -47,7 +47,7 @@ func runApp(args []string) { revel.LoadMimeConfig() // Determine the override port, if any. - port := revel.HttpPort + port := revel.HTTPPort if len(args) == 3 { var err error if port, err = strconv.Atoi(args[2]); err != nil { @@ -61,7 +61,7 @@ func runApp(args []string) { // If the app is run in "watched" mode, use the harness to run it. if revel.Config.BoolDefault("watch", true) && revel.Config.BoolDefault("watch.code", true) { revel.TRACE.Println("Running in watched mode.") - revel.HttpPort = port + revel.HTTPPort = port harness.NewHarness().Run() // Never returns. } diff --git a/revel/test.go b/revel/test.go index 1127ff6..ac01bc2 100644 --- a/revel/test.go +++ b/revel/test.go @@ -114,7 +114,7 @@ You can add it to a run mode configuration with the following line: var ( testSuites []controllers.TestSuiteDesc resp *http.Response - baseUrl = fmt.Sprintf("http://127.0.0.1:%d", revel.HttpPort) + baseUrl = fmt.Sprintf("http://127.0.0.1:%d", revel.HTTPPort) ) for i := 0; ; i++ { if resp, err = http.Get(baseUrl + "/@tests.list"); err == nil {