From 47440bc2ccd01a4b102c147ad89c27da8cebaded Mon Sep 17 00:00:00 2001 From: Denis Isaev Date: Sat, 1 Sep 2018 12:39:22 +0300 Subject: [PATCH] don't print config parsing info logs twice --- pkg/commands/executor.go | 41 +++++++++++++--- pkg/commands/linters.go | 2 +- pkg/commands/root.go | 2 - pkg/commands/run.go | 50 ++++++++++--------- pkg/config/reader.go | 68 ++++++++++++-------------- pkg/lint/lintersdb/enabled_set.go | 1 - pkg/lint/lintersdb/enabled_set_test.go | 2 +- pkg/lint/lintersdb/validator.go | 6 +++ scripts/gen_readme/main.go | 4 +- test/run_test.go | 9 ++-- 10 files changed, 108 insertions(+), 77 deletions(-) diff --git a/pkg/commands/executor.go b/pkg/commands/executor.go index ecd23a33..17a37f4e 100644 --- a/pkg/commands/executor.go +++ b/pkg/commands/executor.go @@ -6,10 +6,12 @@ import ( "github.com/golangci/golangci-lint/pkg/logutils" "github.com/golangci/golangci-lint/pkg/report" "github.com/spf13/cobra" + "github.com/spf13/pflag" ) type Executor struct { rootCmd *cobra.Command + runCmd *cobra.Command exitCode int version, commit, date string @@ -23,22 +25,47 @@ type Executor struct { func NewExecutor(version, commit, date string) *Executor { e := &Executor{ - cfg: config.NewDefault(), - version: version, - commit: commit, - date: date, + cfg: config.NewDefault(), + version: version, + commit: commit, + date: date, + DBManager: lintersdb.NewManager(), } e.log = report.NewLogWrapper(logutils.NewStderrLog(""), &e.reportData) - e.DBManager = lintersdb.NewManager() - e.EnabledLintersSet = lintersdb.NewEnabledSet(e.DBManager, &lintersdb.Validator{}, - e.log.Child("lintersdb"), e.cfg) + // to setup log level early we need to parse config from command line extra time to + // find `-v` option + commandLineCfg, err := e.getConfigForCommandLine() + if err != nil && err != pflag.ErrHelp { + e.log.Fatalf("Can't get config for command line: %s", err) + } + if commandLineCfg != nil { + logutils.SetupVerboseLog(e.log, commandLineCfg.Run.IsVerbose) + } + + // init of commands must be done before config file reading because + // init sets config with the default values of flags e.initRoot() e.initRun() e.initHelp() e.initLinters() + // init e.cfg by values from config: flags parse will see these values + // like the default ones. It will overwrite them only if the same option + // is found in command-line: it's ok, command-line has higher priority. + + r := config.NewFileReader(e.cfg, commandLineCfg, e.log.Child("config_reader")) + if err := r.Read(); err != nil { + e.log.Fatalf("Can't read config: %s", err) + } + + // Slice options must be explicitly set for proper merging of config and command-line options. + fixSlicesFlags(e.runCmd.Flags()) + + e.EnabledLintersSet = lintersdb.NewEnabledSet(e.DBManager, + lintersdb.NewValidator(e.DBManager), e.log.Child("lintersdb"), e.cfg) + return e } diff --git a/pkg/commands/linters.go b/pkg/commands/linters.go index f544e8c0..156bb72f 100644 --- a/pkg/commands/linters.go +++ b/pkg/commands/linters.go @@ -29,7 +29,7 @@ func IsLinterInConfigsList(name string, linters []linter.Config) bool { return false } -func (e Executor) executeLinters(cmd *cobra.Command, args []string) { +func (e *Executor) executeLinters(cmd *cobra.Command, args []string) { enabledLCs, err := e.EnabledLintersSet.Get() if err != nil { log.Fatalf("Can't get enabled linters: %s", err) diff --git a/pkg/commands/root.go b/pkg/commands/root.go index 1ddf2302..1a0c6c08 100644 --- a/pkg/commands/root.go +++ b/pkg/commands/root.go @@ -20,8 +20,6 @@ func (e *Executor) persistentPreRun(cmd *cobra.Command, args []string) { runtime.GOMAXPROCS(e.cfg.Run.Concurrency) - logutils.SetupVerboseLog(e.log, e.cfg.Run.IsVerbose) - if e.cfg.Run.CPUProfilePath != "" { f, err := os.Create(e.cfg.Run.CPUProfilePath) if err != nil { diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 49a49296..06753796 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -168,42 +168,48 @@ func (e *Executor) initRunConfiguration(cmd *cobra.Command) { fs := cmd.Flags() fs.SortFlags = false // sort them as they are defined here initFlagSet(fs, e.cfg, e.DBManager) +} - // init e.cfg by values from config: flags parse will see these values - // like the default ones. It will overwrite them only if the same option - // is found in command-line: it's ok, command-line has higher priority. +func (e Executor) getConfigForCommandLine() (*config.Config, error) { + // We use another pflag.FlagSet here to not set `changed` flag + // on cmd.Flags() options. Otherwise string slice options will be duplicated. + fs := pflag.NewFlagSet("config flag set", pflag.ContinueOnError) - r := config.NewFileReader(e.cfg, e.log.Child("config_reader"), func(fs *pflag.FlagSet, cfg *config.Config) { - // Don't do `fs.AddFlagSet(cmd.Flags())` because it shares flags representations: - // `changed` variable inside string slice vars will be shared. - // Use another config variable here, not e.cfg, to not - // affect main parsing by this parsing of only config option. - initFlagSet(fs, cfg, e.DBManager) + var cfg config.Config + // Don't do `fs.AddFlagSet(cmd.Flags())` because it shares flags representations: + // `changed` variable inside string slice vars will be shared. + // Use another config variable here, not e.cfg, to not + // affect main parsing by this parsing of only config option. + initFlagSet(fs, &cfg, e.DBManager) - // Parse max options, even force version option: don't want - // to get access to Executor here: it's error-prone to use - // cfg vs e.cfg. - initRootFlagSet(fs, cfg, true) - }) - if err := r.Read(); err != nil { - e.log.Fatalf("Can't read config: %s", err) + // Parse max options, even force version option: don't want + // to get access to Executor here: it's error-prone to use + // cfg vs e.cfg. + initRootFlagSet(fs, &cfg, true) + + fs.Usage = func() {} // otherwise help text will be printed twice + if err := fs.Parse(os.Args); err != nil { + if err == pflag.ErrHelp { + return nil, err + } + + return nil, fmt.Errorf("can't parse args: %s", err) } - // Slice options must be explicitly set for proper merging of config and command-line options. - fixSlicesFlags(fs) + return &cfg, nil } func (e *Executor) initRun() { - var runCmd = &cobra.Command{ + e.runCmd = &cobra.Command{ Use: "run", Short: welcomeMessage, Run: e.executeRun, } - e.rootCmd.AddCommand(runCmd) + e.rootCmd.AddCommand(e.runCmd) - runCmd.SetOutput(logutils.StdOut) // use custom output to properly color it in Windows terminals + e.runCmd.SetOutput(logutils.StdOut) // use custom output to properly color it in Windows terminals - e.initRunConfiguration(runCmd) + e.initRunConfiguration(e.runCmd) } func fixSlicesFlags(fs *pflag.FlagSet) { diff --git a/pkg/config/reader.go b/pkg/config/reader.go index a837ff48..ac978ad5 100644 --- a/pkg/config/reader.go +++ b/pkg/config/reader.go @@ -5,26 +5,24 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/golangci/golangci-lint/pkg/fsutils" "github.com/golangci/golangci-lint/pkg/logutils" - "github.com/spf13/pflag" "github.com/spf13/viper" ) -type FlagSetInit func(fs *pflag.FlagSet, cfg *Config) - type FileReader struct { - log logutils.Log - cfg *Config - flagSetInit FlagSetInit + log logutils.Log + cfg *Config + commandLineCfg *Config } -func NewFileReader(toCfg *Config, log logutils.Log, flagSetInit FlagSetInit) *FileReader { +func NewFileReader(toCfg, commandLineCfg *Config, log logutils.Log) *FileReader { return &FileReader{ - log: log, - cfg: toCfg, - flagSetInit: flagSetInit, + log: log, + cfg: toCfg, + commandLineCfg: commandLineCfg, } } @@ -33,9 +31,9 @@ func (r *FileReader) Read() error { // 1. to access "config" option here. // 2. to give config less priority than command line. - configFile, restArgs, err := r.parseConfigOption() + configFile, err := r.parseConfigOption() if err != nil { - if err == errConfigDisabled || err == pflag.ErrHelp { + if err == errConfigDisabled { return nil } @@ -45,7 +43,7 @@ func (r *FileReader) Read() error { if configFile != "" { viper.SetConfigFile(configFile) } else { - r.setupConfigFileSearch(restArgs) + r.setupConfigFileSearch() } return r.parseConfig() @@ -108,7 +106,9 @@ func (r *FileReader) validateConfig() error { return nil } -func (r *FileReader) setupConfigFileSearch(args []string) { +func getFirstPathArg() string { + args := os.Args + // skip all args ([golangci-lint, run/linters]) before files/dirs list for len(args) != 0 { if args[0] == "run" { @@ -121,10 +121,18 @@ func (r *FileReader) setupConfigFileSearch(args []string) { // find first file/dir arg firstArg := "./..." - if len(args) != 0 { - firstArg = args[0] + for _, arg := range args { + if !strings.HasPrefix(arg, "-") { + firstArg = arg + break + } } + return firstArg +} + +func (r *FileReader) setupConfigFileSearch() { + firstArg := getFirstPathArg() absStartPath, err := filepath.Abs(firstArg) if err != nil { r.log.Warnf("Can't make abs path for %q: %s", firstArg, err) @@ -159,34 +167,20 @@ func (r *FileReader) setupConfigFileSearch(args []string) { var errConfigDisabled = errors.New("config is disabled by --no-config") -func (r *FileReader) parseConfigOption() (string, []string, error) { - // We use another pflag.FlagSet here to not set `changed` flag - // on cmd.Flags() options. Otherwise string slice options will be duplicated. - fs := pflag.NewFlagSet("config flag set", pflag.ContinueOnError) - - var cfg Config - r.flagSetInit(fs, &cfg) - - fs.Usage = func() {} // otherwise help text will be printed twice - if err := fs.Parse(os.Args); err != nil { - if err == pflag.ErrHelp { - return "", nil, err - } - - return "", nil, fmt.Errorf("can't parse args: %s", err) +func (r *FileReader) parseConfigOption() (string, error) { + cfg := r.commandLineCfg + if cfg == nil { + return "", nil } - // for `-v` to work until running of preRun function - logutils.SetupVerboseLog(r.log, cfg.Run.IsVerbose) - configFile := cfg.Run.Config if cfg.Run.NoConfig && configFile != "" { - return "", nil, fmt.Errorf("can't combine option --config and --no-config") + return "", fmt.Errorf("can't combine option --config and --no-config") } if cfg.Run.NoConfig { - return "", nil, errConfigDisabled + return "", errConfigDisabled } - return configFile, fs.Args(), nil + return configFile, nil } diff --git a/pkg/lint/lintersdb/enabled_set.go b/pkg/lint/lintersdb/enabled_set.go index 14d36168..fb969f7c 100644 --- a/pkg/lint/lintersdb/enabled_set.go +++ b/pkg/lint/lintersdb/enabled_set.go @@ -129,7 +129,6 @@ func (es EnabledSet) Get() ([]linter.Config, error) { } es.verbosePrintLintersStatus(resultLinters) - return resultLinters, nil } diff --git a/pkg/lint/lintersdb/enabled_set_test.go b/pkg/lint/lintersdb/enabled_set_test.go index 33365171..49335535 100644 --- a/pkg/lint/lintersdb/enabled_set_test.go +++ b/pkg/lint/lintersdb/enabled_set_test.go @@ -45,7 +45,7 @@ func TestGetEnabledLintersSet(t *testing.T) { } m := NewManager() - es := NewEnabledSet(m, &Validator{}, nil, nil) + es := NewEnabledSet(m, NewValidator(m), nil, nil) for _, c := range cases { t.Run(c.name, func(t *testing.T) { defaultLinters := []linter.Config{} diff --git a/pkg/lint/lintersdb/validator.go b/pkg/lint/lintersdb/validator.go index 4ccc5726..7391fa50 100644 --- a/pkg/lint/lintersdb/validator.go +++ b/pkg/lint/lintersdb/validator.go @@ -11,6 +11,12 @@ type Validator struct { m *Manager } +func NewValidator(m *Manager) *Validator { + return &Validator{ + m: m, + } +} + func (v Validator) validateLintersNames(cfg *config.Linters) error { allNames := append([]string{}, cfg.Enable...) allNames = append(allNames, cfg.Disable...) diff --git a/scripts/gen_readme/main.go b/scripts/gen_readme/main.go index a11e1e3d..dc6bc64d 100644 --- a/scripts/gen_readme/main.go +++ b/scripts/gen_readme/main.go @@ -89,7 +89,7 @@ func buildTemplateContext() (map[string]interface{}, error) { func getLintersListMarkdown(enabled bool) string { var neededLcs []linter.Config - lcs := lintersdb.GetAllSupportedLinterConfigs() + lcs := lintersdb.NewManager().GetAllSupportedLinterConfigs() for _, lc := range lcs { if lc.EnabledByDefault == enabled { neededLcs = append(neededLcs, lc) @@ -114,7 +114,7 @@ func getLintersListMarkdown(enabled bool) string { func getThanksList() string { var lines []string addedAuthors := map[string]bool{} - for _, lc := range lintersdb.GetAllSupportedLinterConfigs() { + for _, lc := range lintersdb.NewManager().GetAllSupportedLinterConfigs() { if lc.OriginalURL == "" { continue } diff --git a/test/run_test.go b/test/run_test.go index ae370be2..eec04838 100644 --- a/test/run_test.go +++ b/test/run_test.go @@ -170,7 +170,8 @@ func inSlice(s []string, v string) bool { } func getEnabledByDefaultFastLintersExcept(except ...string) []string { - ebdl := lintersdb.GetAllEnabledByDefaultLinters() + m := lintersdb.NewManager() + ebdl := m.GetAllEnabledByDefaultLinters() ret := []string{} for _, linter := range ebdl { if linter.DoesFullImport { @@ -186,7 +187,7 @@ func getEnabledByDefaultFastLintersExcept(except ...string) []string { } func getAllFastLintersWith(with ...string) []string { - linters := lintersdb.GetAllSupportedLinterConfigs() + linters := lintersdb.NewManager().GetAllSupportedLinterConfigs() ret := append([]string{}, with...) for _, linter := range linters { if linter.DoesFullImport { @@ -199,7 +200,7 @@ func getAllFastLintersWith(with ...string) []string { } func getEnabledByDefaultLinters() []string { - ebdl := lintersdb.GetAllEnabledByDefaultLinters() + ebdl := lintersdb.NewManager().GetAllEnabledByDefaultLinters() ret := []string{} for _, linter := range ebdl { ret = append(ret, linter.Linter.Name()) @@ -209,7 +210,7 @@ func getEnabledByDefaultLinters() []string { } func getEnabledByDefaultFastLintersWith(with ...string) []string { - ebdl := lintersdb.GetAllEnabledByDefaultLinters() + ebdl := lintersdb.NewManager().GetAllEnabledByDefaultLinters() ret := append([]string{}, with...) for _, linter := range ebdl { if linter.DoesFullImport {