Skip to content

Commit

Permalink
Handle problems detected by used linters
Browse files Browse the repository at this point in the history
The problems in the code base detected by the linters that have been
integrated in GH-62 through GolangCI have been handled by refactoring
the affected implementations.
This helps to improve the overall code quality and prevents possible
errors.

1. Removed unused function parameters detected by unparam (1).
   1. `(*cmdOptions).prepare` - `cmd` is unused:
      cmd/snowsaw/bootstrap/bootstrap.go:51:30 (2)
      ```go
      func (o *cmdOptions) prepare(cmd *cobra.Command, args []string) {
                                   ^
      ```
   2. `(*cmdOptions).run` - `cmd` is unused:
      cmd/snowsaw/bootstrap/bootstrap.go:100:26 (2)
      ```go
      func (o *cmdOptions) run(cmd *cobra.Command, args []string) {
                               ^
      ```
   3. `(*cmdOptions).run` - `args` is unused:
      cmd/snowsaw/bootstrap/bootstrap.go:100:46 (2)
      ```go
      func (o *cmdOptions) run(cmd *cobra.Command, args []string) {
                                                   ^
      ```
2. Improved function names and code flows detected by golint (3).
   1. func `NewJsonEncoder` should be `NewJSONEncoder`:
      pkg/config/encoder/json/json.go:34:6 (4)
      ```go
      func NewJsonEncoder() Encoder {
           ^
      ```
   2. var `ExtensionsJson` should be `ExtensionsJSON`:
   pkg/config/encoder/constants.go:26:2 (5)
      ```go
      ExtensionsJson = "json"
      ^
      ```
   3. if block ends with a return statement, so drop this else and
      outdent its block (move short variable declaration to its own line
      if necessary): pkg/prt/printer.go:121:9 (6)
      ```go
      } else {
             ^
      ```
   4. exported func Load returns unexported type *builder.builder, which
      can be annoying to use: pkg/config/builder/builder.go:39:32 (7)
      ```go
      func Load(files ...*file.File) *builder {
                                     ^
      ```
3. Improved code style smells detected by gocritic (8).
   1. assignOp: replace `format = format + "\n"` with `format += "\n"`:
   pkg/prt/printer.go:179:4 (9)
      ```go
      format = format + "\n"
      ^
      ```
   2. paramTypeCombine: `func(v Verbosity, w io.Writer, prefix string,
      format string, args ...interface{})` could be replaced with
      `func(v Verbosity, w io.Writer, prefix, format string, args
      ...interface{})`: pkg/prt/printer.go:176:1 (10)
      ```go
      func (p *printerConfig) withNewLine(v Verbosity, w io.Writer,
      ^
      prefix string, format string, args ...interface{}) {
      ```
   3. emptyStringTest: replace `len(parts[0]) == 0` with `parts[0] ==
      ""`: pkg/snowblock/task/shell/shell.go:165:5 (11)
      ```go
      if len(parts[0]) == 0 {
         ^
      ```
   4. elseif: can replace 'else {if cond {}}' with 'else if cond {}':
   cmd/snowsaw/bootstrap/bootstrap.go:57:9 (12)
      ```go
      } else {
             ^
      ```
4. Remove unnecessary type conversions detected by unconvert (13).
   1. unnecessary conversion: pkg/prt/printer.go:132:16 (14)
      ```go
      *v = Verbosity(l)
                    ^
      ```

References:
  (1) https://github.com/mvdan/unparam
  (2) https://github.com/arcticicestudio/snowsaw/blob/9366c4a9c6d59dd0fccad12fbc413842ea751fa6/cmd/snowsaw/bootstrap/bootstrap.go#L51
  (3) https://github.com/golang/lint
  (4) https://github.com/arcticicestudio/snowsaw/blob/5aa483e7e5e45888254aa4d0143d2afb898b4332/pkg/config/encoder/json/json.go#L34
  (5) https://github.com/arcticicestudio/snowsaw/blob/008edbcb509af2cb5ced942d679fa3845a4ec1e1/pkg/config/encoder/constants.go#L26
  (6) https://github.com/arcticicestudio/snowsaw/blob/79afc12ebc15620fd94e78416e0b49a68bbf2eb6/pkg/prt/printer.go#L121
  (7) https://github.com/arcticicestudio/snowsaw/blob/dea6ab56b7410a8cbc8901818703d5ab1ace5c87/pkg/config/builder/builder.go#L39
  (8) https://github.com/go-critic/go-critic
  (9) https://github.com/arcticicestudio/snowsaw/blob/79afc12ebc15620fd94e78416e0b49a68bbf2eb6/pkg/prt/printer.go#L179
  (10) https://github.com/arcticicestudio/snowsaw/blob/79afc12ebc15620fd94e78416e0b49a68bbf2eb6/pkg/prt/printer.go#L176
  (11) https://github.com/arcticicestudio/snowsaw/blob/a78810b7ccb5ddb8e80929d54fb7c461a1b80a1c/pkg/snowblock/task/shell/shell.go#L165
  (12) https://github.com/arcticicestudio/snowsaw/blob/9366c4a9c6d59dd0fccad12fbc413842ea751fa6/cmd/snowsaw/bootstrap/bootstrap.go#L57
  (13) https://github.com/mdempsky/unconvert
  (14) https://github.com/arcticicestudio/snowsaw/blob/79afc12ebc15620fd94e78416e0b49a68bbf2eb6/pkg/prt/printer.go#L132

Epic: GH-33
Resolves GH-80
  • Loading branch information
arcticicestudio committed Jul 20, 2019
1 parent a78810b commit 8136905
Show file tree
Hide file tree
Showing 8 changed files with 25 additions and 27 deletions.
16 changes: 7 additions & 9 deletions cmd/snowsaw/bootstrap/bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,24 +41,22 @@ func NewBootstrapCmd() *cobra.Command {
To process individual snowblocks a list of space-separated paths can be passed as arguments.
`,
Run: func(cmd *cobra.Command, args []string) {
o.prepare(cmd, args)
o.run(cmd, args)
o.prepare(args)
o.run()
},
}
return bootstrapCmd
}

func (o *cmdOptions) prepare(cmd *cobra.Command, args []string) {
func (o *cmdOptions) prepare(args []string) {
// Use explicit snowblocks if specified, otherwise find all snowblocks within the base directories.
if len(args) > 0 {
prt.Debugf("Using individual snowblocks instead of configured base directories(s): %s",
color.CyanString("%v", args))
config.AppConfig.Snowblocks.Paths = args
} else {
if err := o.readSnowblockDirectories(); err != nil {
prt.Errorf("Failed to read snowblocks from base directories: %v", err)
os.Exit(1)
}
} else if err := o.readSnowblockDirectories(); err != nil {
prt.Errorf("Failed to read snowblocks from base directories: %v", err)
os.Exit(1)
}
o.SnowblockPaths = config.AppConfig.Snowblocks.Paths
}
Expand Down Expand Up @@ -97,7 +95,7 @@ func (o *cmdOptions) readSnowblockDirectories() error {
return nil
}

func (o *cmdOptions) run(cmd *cobra.Command, args []string) {
func (o *cmdOptions) run() {
for _, path := range o.SnowblockPaths {
sb := snowblock.NewSnowblock(path)
err := sb.Validate(config.SnowblockTaskRunnerRegistry.GetAll())
Expand Down
1 change: 1 addition & 0 deletions magefile.go
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ func runGolangCILint() {
prt.Errorf("Linters finished with non-zero exit code")
os.Exit(1)
}
prt.Successf("Linters finished successfully with zero exit code")
}

func runGoTest(testFlags ...string) {
Expand Down
10 changes: 5 additions & 5 deletions pkg/config/builder/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,16 @@ import (
"github.com/arcticicestudio/snowsaw/pkg/util/filesystem"
)

// builder contains the current configuration building state.
type builder struct {
// Builder contains the current configuration building state.
type Builder struct {
Files []*file.File
}

// Load tries to load all given configuration files.
// It checks if the path is valid and exists, tries to assign a matching encoder based on the file extension and returns
// a pointer to a builder to chain the merge function.
func Load(files ...*file.File) *builder {
b := &builder{Files: []*file.File{}}
func Load(files ...*file.File) *Builder {
b := &Builder{Files: []*file.File{}}

for _, f := range files {
// Convert to an absolute path and check if the file exists, otherwise ignore and check next.
Expand Down Expand Up @@ -70,7 +70,7 @@ func Load(files ...*file.File) *builder {

// Into accepts a configuration struct pointer and populates it with the current config state.
// The order of the files are maintained when merging of the configuration states is enabled.
func (b *builder) Into(c *config.Config, merge bool) error {
func (b *Builder) Into(c *config.Config, merge bool) error {
for _, f := range b.Files {
content, err := ioutil.ReadFile(f.Path)
if err != nil {
Expand Down
6 changes: 3 additions & 3 deletions pkg/config/encoder/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ import (
var (
// ExtensionMapping maps supported file extensions to their compatible encoders.
ExtensionMapping = map[string]Encoder{
ExtensionsJson: json.NewJsonEncoder(),
ExtensionsJSON: json.NewJSONEncoder(),
ExtensionsYaml: yaml.NewYamlEncoder(),
}
// ExtensionsJson is the supported extension for files containing JSON data.
ExtensionsJson = "json"
// ExtensionsJSON is the supported extension for files containing JSON data.
ExtensionsJSON = "json"
// ExtensionsYaml is the supported extension for files containing YAML data.
ExtensionsYaml = "yml"
)
4 changes: 2 additions & 2 deletions pkg/config/encoder/json/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func (j Encoder) Decode(d []byte, v interface{}) error {
return json.Unmarshal(d, v)
}

// NewJsonEncoder returns a new JSON Encoder.
func NewJsonEncoder() Encoder {
// NewJSONEncoder returns a new JSON Encoder.
func NewJSONEncoder() Encoder {
return Encoder{}
}
9 changes: 4 additions & 5 deletions pkg/prt/printer.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,8 @@ func (v Verbosity) MarshalText() ([]byte, error) {
func (v Verbosity) String() string {
if b, err := v.MarshalText(); err == nil {
return string(b)
} else {
return "unknown"
}
return "unknown"
}

// UnmarshalText implements encoding.TextUnmarshaler to unmarshal a textual representation of itself.
Expand All @@ -130,7 +129,7 @@ func (v *Verbosity) UnmarshalText(text []byte) error {
return err
}

*v = Verbosity(l)
*v = l
return nil
}

Expand Down Expand Up @@ -174,10 +173,10 @@ func (p *printerConfig) isPrinterEnabled(v Verbosity) bool { return p.verbosity
func (p *printerConfig) setVerbosityLevel(v Verbosity) { p.verbosity = v }

// withNewLine writes to the specified writer and appends a new line to the given format if not already.
func (p *printerConfig) withNewLine(v Verbosity, w io.Writer, prefix string, format string, args ...interface{}) {
func (p *printerConfig) withNewLine(v Verbosity, w io.Writer, prefix, format string, args ...interface{}) {
if p.isPrinterEnabled(v) {
if !strings.HasSuffix(format, "\n") {
format = format + "\n"
format += "\n"
}
fmt.Fprintf(w, prefix+format, args...)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/snowblock/snowblock.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func (s *Snowblock) Validate(taskRunner map[string]api.TaskRunner) error {
s.Path = expandedAbsPath

// Try to read and encode the task objects when the directory contains a configuration file.
configFilePath := filepath.Join(s.Path, fmt.Sprintf("%s.%s", api.ConfigurationFileName, encoder.ExtensionsJson))
configFilePath := filepath.Join(s.Path, fmt.Sprintf("%s.%s", api.ConfigurationFileName, encoder.ExtensionsJSON))
if configLoadErr := loadConfigFile(configFilePath, &s.TaskObjects); configLoadErr != nil {
prt.Debugf("Ignoring snowblock directory %s: %v",
color.CyanString(filepath.Base(s.Path)), configLoadErr)
Expand All @@ -124,7 +124,7 @@ func (s *Snowblock) Validate(taskRunner map[string]api.TaskRunner) error {
}

func loadConfigFile(absPath string, tasks *[]api.Task) error {
f := file.NewFile(absPath).WithEncoder(json.NewJsonEncoder())
f := file.NewFile(absPath).WithEncoder(json.NewJSONEncoder())
// Check if the file exists...
if exists, _ := filesystem.FileExists(f.Path); !exists {
return fmt.Errorf("no such snowblock configuration file: %s", color.RedString(f.Path))
Expand Down
2 changes: 1 addition & 1 deletion pkg/snowblock/task/shell/shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func (s *Shell) execute() error {

func (s *Shell) parseCommand(cmd string) error {
parts := strings.Split(strings.TrimSpace(cmd), " ")
if len(parts[0]) == 0 {
if parts[0] == "" {
return fmt.Errorf("shell command must not be empty or whitespace-only")
}

Expand Down

0 comments on commit 8136905

Please sign in to comment.