Skip to content

dev: clean code arround CLI args usage #4557

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

Merged
merged 10 commits into from
Mar 28, 2024
4 changes: 2 additions & 2 deletions pkg/commands/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func newConfigCommand(log logutils.Log, info BuildInfo) *configCommand {
return c
}

func (c *configCommand) preRunE(cmd *cobra.Command, _ []string) error {
func (c *configCommand) preRunE(cmd *cobra.Command, args []string) error {
// The command doesn't depend on the real configuration.
// It only needs to know the path of the configuration file.
cfg := config.NewDefault()
Expand All @@ -89,7 +89,7 @@ func (c *configCommand) preRunE(cmd *cobra.Command, _ []string) error {
// TODO(ldez) add an option (check deprecation) to `Loader.Load()` but this require a dedicated PR.
cfg.Run.UseDefaultSkipDirs = true

loader := config.NewLoader(c.log.Child(logutils.DebugKeyConfigReader), c.viper, cmd.Flags(), c.opts, cfg)
loader := config.NewLoader(c.log.Child(logutils.DebugKeyConfigReader), c.viper, cmd.Flags(), c.opts, cfg, args)

if err := loader.Load(); err != nil {
return fmt.Errorf("can't load config: %w", err)
Expand Down
4 changes: 2 additions & 2 deletions pkg/commands/linters.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,15 @@ func newLintersCommand(logger logutils.Log) *lintersCommand {
return c
}

func (c *lintersCommand) preRunE(cmd *cobra.Command, _ []string) error {
func (c *lintersCommand) preRunE(cmd *cobra.Command, args []string) error {
// Hack to hide deprecation messages related to `--skip-dirs-use-default`:
// Flags are not bound then the default values, defined only through flags, are not applied.
// In this command, linters information are the only requirements, i.e. it don't need flag values.
//
// TODO(ldez) add an option (check deprecation) to `Loader.Load()` but this require a dedicated PR.
c.cfg.Run.UseDefaultSkipDirs = true

loader := config.NewLoader(c.log.Child(logutils.DebugKeyConfigReader), c.viper, cmd.Flags(), c.opts.LoaderOptions, c.cfg)
loader := config.NewLoader(c.log.Child(logutils.DebugKeyConfigReader), c.viper, cmd.Flags(), c.opts.LoaderOptions, c.cfg, args)

if err := loader.Load(); err != nil {
return fmt.Errorf("can't load config: %w", err)
Expand Down
4 changes: 2 additions & 2 deletions pkg/commands/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,12 @@ func newRunCommand(logger logutils.Log, info BuildInfo) *runCommand {
return c
}

func (c *runCommand) persistentPreRunE(cmd *cobra.Command, _ []string) error {
func (c *runCommand) persistentPreRunE(cmd *cobra.Command, args []string) error {
if err := c.startTracing(); err != nil {
return err
}

loader := config.NewLoader(c.log.Child(logutils.DebugKeyConfigReader), c.viper, cmd.Flags(), c.opts.LoaderOptions, c.cfg)
loader := config.NewLoader(c.log.Child(logutils.DebugKeyConfigReader), c.viper, cmd.Flags(), c.opts.LoaderOptions, c.cfg, args)

if err := loader.Load(); err != nil {
return fmt.Errorf("can't load config: %w", err)
Expand Down
85 changes: 35 additions & 50 deletions pkg/config/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"os"
"path/filepath"
"slices"
"strings"

"github.com/go-viper/mapstructure/v2"
"github.com/mitchellh/go-homedir"
Expand All @@ -33,16 +32,18 @@ type Loader struct {

log logutils.Log

cfg *Config
cfg *Config
args []string
}

func NewLoader(log logutils.Log, v *viper.Viper, fs *pflag.FlagSet, opts LoaderOptions, cfg *Config) *Loader {
func NewLoader(log logutils.Log, v *viper.Viper, fs *pflag.FlagSet, opts LoaderOptions, cfg *Config, args []string) *Loader {
return &Loader{
opts: opts,
viper: v,
fs: fs,
log: log,
cfg: cfg,
args: args,
}
}

Expand Down Expand Up @@ -116,50 +117,59 @@ func (l *Loader) evaluateOptions() (string, error) {
}

func (l *Loader) setupConfigFileSearch() {
firstArg := extractFirstPathArg()
l.viper.SetConfigName(".golangci")

configSearchPaths := l.getConfigSearchPaths()

l.log.Infof("Config search paths: %s", configSearchPaths)

for _, p := range configSearchPaths {
l.viper.AddConfigPath(p)
}
}

func (l *Loader) getConfigSearchPaths() []string {
firstArg := "./..."
if len(l.args) > 0 {
firstArg = l.args[0]
}

absStartPath, err := filepath.Abs(firstArg)
absPath, err := filepath.Abs(firstArg)
if err != nil {
l.log.Warnf("Can't make abs path for %q: %s", firstArg, err)
absStartPath = filepath.Clean(firstArg)
absPath = filepath.Clean(firstArg)
}

// start from it
var curDir string
if fsutils.IsDir(absStartPath) {
curDir = absStartPath
var currentDir string
if fsutils.IsDir(absPath) {
currentDir = absPath
} else {
curDir = filepath.Dir(absStartPath)
currentDir = filepath.Dir(absPath)
}

// find all dirs from it up to the root
configSearchPaths := []string{"./"}
searchPaths := []string{"./"}

for {
configSearchPaths = append(configSearchPaths, curDir)
searchPaths = append(searchPaths, currentDir)

newCurDir := filepath.Dir(curDir)
if curDir == newCurDir || newCurDir == "" {
parent := filepath.Dir(currentDir)
if currentDir == parent || parent == "" {
break
}

curDir = newCurDir
currentDir = parent
}

// find home directory for global config
if home, err := homedir.Dir(); err != nil {
l.log.Warnf("Can't get user's home directory: %s", err.Error())
} else if !slices.Contains(configSearchPaths, home) {
configSearchPaths = append(configSearchPaths, home)
l.log.Warnf("Can't get user's home directory: %v", err)
} else if !slices.Contains(searchPaths, home) {
searchPaths = append(searchPaths, home)
}

l.log.Infof("Config search paths: %s", configSearchPaths)

l.viper.SetConfigName(".golangci")

for _, p := range configSearchPaths {
l.viper.AddConfigPath(p)
}
return searchPaths
}

func (l *Loader) parseConfig() error {
Expand Down Expand Up @@ -416,28 +426,3 @@ func customDecoderHook() viper.DecoderConfigOption {
mapstructure.TextUnmarshallerHookFunc(),
))
}

func extractFirstPathArg() string {
args := os.Args

// skip all args ([golangci-lint, run/linters]) before files/dirs list
for len(args) != 0 {
if args[0] == "run" {
args = args[1:]
break
}

args = args[1:]
}

// find first file/dir arg
firstArg := "./..."
for _, arg := range args {
if !strings.HasPrefix(arg, "-") {
firstArg = arg
break
}
}

return firstArg
}
40 changes: 21 additions & 19 deletions pkg/lint/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,10 @@ func (l *PackageLoader) loadPackages(ctx context.Context, loadMode packages.Load
// TODO: use fset, parsefile, overlay
}

args := l.buildArgs()
args := buildArgs(l.args)

l.debugf("Built loader args are %s", args)

pkgs, err := packages.Load(conf, args...)
if err != nil {
return nil, fmt.Errorf("failed to load with go/packages: %w", err)
Expand Down Expand Up @@ -212,24 +214,6 @@ func (l *PackageLoader) prepareBuildContext() {
build.Default.BuildTags = l.cfg.Run.BuildTags
}

func (l *PackageLoader) buildArgs() []string {
if len(l.args) == 0 {
return []string{"./..."}
}

var retArgs []string
for _, arg := range l.args {
if strings.HasPrefix(arg, ".") || filepath.IsAbs(arg) {
retArgs = append(retArgs, arg)
} else {
// go/packages doesn't work well if we don't have the prefix ./ for local packages
retArgs = append(retArgs, fmt.Sprintf(".%c%s", filepath.Separator, arg))
}
}

return retArgs
}

func (l *PackageLoader) makeBuildFlags() []string {
var buildFlags []string

Expand All @@ -247,6 +231,24 @@ func (l *PackageLoader) makeBuildFlags() []string {
return buildFlags
}

func buildArgs(args []string) []string {
if len(args) == 0 {
return []string{"./..."}
}

var retArgs []string
for _, arg := range args {
if strings.HasPrefix(arg, ".") || filepath.IsAbs(arg) {
retArgs = append(retArgs, arg)
} else {
// go/packages doesn't work well if we don't have the prefix ./ for local packages
retArgs = append(retArgs, fmt.Sprintf(".%c%s", filepath.Separator, arg))
}
}

return retArgs
}

func findLoadMode(linters []*linter.Config) packages.LoadMode {
loadMode := packages.LoadMode(0)
for _, lc := range linters {
Expand Down
58 changes: 58 additions & 0 deletions pkg/lint/package_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package lint

import (
"path/filepath"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func Test_buildArgs(t *testing.T) {
testCases := []struct {
desc string
args []string
expected []string
}{
{
desc: "empty",
args: nil,
expected: []string{"./..."},
},
{
desc: "start with a dot",
args: []string{filepath.FromSlash("./foo")},
expected: []string{filepath.FromSlash("./foo")},
},
{
desc: "start without a dot",
args: []string{"foo"},
expected: []string{filepath.FromSlash("./foo")},
},
{
desc: "absolute path",
args: []string{mustAbs(t, "/tmp/foo")},
expected: []string{mustAbs(t, "/tmp/foo")},
},
}

for _, test := range testCases {
test := test
t.Run(test.desc, func(t *testing.T) {
t.Parallel()

results := buildArgs(test.args)

assert.Equal(t, test.expected, results)
})
}
}

func mustAbs(t *testing.T, p string) string {
t.Helper()

abs, err := filepath.Abs(filepath.FromSlash(p))
require.NoError(t, err)

return abs
}
5 changes: 0 additions & 5 deletions pkg/result/processors/autogenerated_exclude.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"fmt"
"go/parser"
"go/token"
"path/filepath"
"regexp"
"strings"

Expand Down Expand Up @@ -157,7 +156,3 @@ func getComments(filePath string) (string, error) {

return strings.Join(docLines, "\n"), nil
}

func isGoFile(name string) bool {
return filepath.Ext(name) == ".go"
}
4 changes: 4 additions & 0 deletions pkg/result/processors/invalid_issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,7 @@ func (p InvalidIssue) shouldPassIssue(issue *result.Issue) (bool, error) {

return true, nil
}

func isGoFile(name string) bool {
return filepath.Ext(name) == ".go"
}
Loading