From a1d049c1c4b8986a975b7d8492764701503f9ed8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Thu, 31 Oct 2024 20:52:39 +0200 Subject: [PATCH] internal/flags: remove low-use type TextMarshalerFlag (#30707) Currently we have a custom TextMarshalerFlag. It's a nice idea, allowing anything implementing text marshaller to be used as a flag. That said, we only ever used it in one place because it's not that obvious how to use and it needs some boilerplate on the type itself too, apart of the heavy boilerplate got the custom flag. All in all there's no *need* to drop this feature just now, but while porting the cmds over to cli @v3, all other custom flags worker perfectly, whereas this one started crashing deep inside the cli package. The flag handling in v3 got rebuild on generics and there are a number of new methods needed; and my guess is that maybe one of them doesn't work like this flag currently is designed too. We could definitely try and redesign this flag for cli v3... but all that effort and boilerplate just to use it for 1 flag in 1 location, seems not worth it. So for now I'm suggesting removing it and maybe reconsider a similar feature in cli v3 with however it will work. --- cmd/utils/flags.go | 11 ++-- internal/flags/flags.go | 114 -------------------------------------- internal/flags/helpers.go | 3 - 3 files changed, 6 insertions(+), 122 deletions(-) diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index be8aa34140..7a4effdf1a 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -211,8 +211,7 @@ var ( Value: 0, } - defaultSyncMode = ethconfig.Defaults.SyncMode - SnapshotFlag = &cli.BoolFlag{ + SnapshotFlag = &cli.BoolFlag{ Name: "snapshot", Usage: `Enables snapshot-database mode (default = enable)`, Value: true, @@ -244,10 +243,10 @@ var ( Usage: "Manually specify the Verkle fork timestamp, overriding the bundled setting", Category: flags.EthCategory, } - SyncModeFlag = &flags.TextMarshalerFlag{ + SyncModeFlag = &cli.StringFlag{ Name: "syncmode", Usage: `Blockchain sync mode ("snap" or "full")`, - Value: &defaultSyncMode, + Value: ethconfig.Defaults.SyncMode.String(), Category: flags.StateCategory, } GCModeFlag = &cli.StringFlag{ @@ -1669,7 +1668,9 @@ func SetEthConfig(ctx *cli.Context, stack *node.Node, cfg *ethconfig.Config) { if ctx.IsSet(SyncTargetFlag.Name) { cfg.SyncMode = downloader.FullSync // dev sync target forces full sync } else if ctx.IsSet(SyncModeFlag.Name) { - cfg.SyncMode = *flags.GlobalTextMarshaler(ctx, SyncModeFlag.Name).(*downloader.SyncMode) + if err = cfg.SyncMode.UnmarshalText([]byte(ctx.String(SyncModeFlag.Name))); err != nil { + Fatalf("invalid --syncmode flag: %v", err) + } } if ctx.IsSet(NetworkIdFlag.Name) { cfg.NetworkId = ctx.Uint64(NetworkIdFlag.Name) diff --git a/internal/flags/flags.go b/internal/flags/flags.go index bf62c53adf..1f6be3d3d3 100644 --- a/internal/flags/flags.go +++ b/internal/flags/flags.go @@ -17,7 +17,6 @@ package flags import ( - "encoding" "errors" "flag" "fmt" @@ -122,119 +121,6 @@ func (f *DirectoryFlag) GetDefaultText() string { return f.GetValue() } -type TextMarshaler interface { - encoding.TextMarshaler - encoding.TextUnmarshaler -} - -// textMarshalerVal turns a TextMarshaler into a flag.Value -type textMarshalerVal struct { - v TextMarshaler -} - -func (v textMarshalerVal) String() string { - if v.v == nil { - return "" - } - text, _ := v.v.MarshalText() - return string(text) -} - -func (v textMarshalerVal) Set(s string) error { - return v.v.UnmarshalText([]byte(s)) -} - -var ( - _ cli.Flag = (*TextMarshalerFlag)(nil) - _ cli.RequiredFlag = (*TextMarshalerFlag)(nil) - _ cli.VisibleFlag = (*TextMarshalerFlag)(nil) - _ cli.DocGenerationFlag = (*TextMarshalerFlag)(nil) - _ cli.CategorizableFlag = (*TextMarshalerFlag)(nil) -) - -// TextMarshalerFlag wraps a TextMarshaler value. -type TextMarshalerFlag struct { - Name string - - Category string - DefaultText string - Usage string - - Required bool - Hidden bool - HasBeenSet bool - - Value TextMarshaler - - Aliases []string - EnvVars []string -} - -// For cli.Flag: - -func (f *TextMarshalerFlag) Names() []string { return append([]string{f.Name}, f.Aliases...) } -func (f *TextMarshalerFlag) IsSet() bool { return f.HasBeenSet } -func (f *TextMarshalerFlag) String() string { return cli.FlagStringer(f) } - -func (f *TextMarshalerFlag) Apply(set *flag.FlagSet) error { - for _, envVar := range f.EnvVars { - envVar = strings.TrimSpace(envVar) - if value, found := syscall.Getenv(envVar); found { - if err := f.Value.UnmarshalText([]byte(value)); err != nil { - return fmt.Errorf("could not parse %q from environment variable %q for flag %s: %s", value, envVar, f.Name, err) - } - f.HasBeenSet = true - break - } - } - eachName(f, func(name string) { - set.Var(textMarshalerVal{f.Value}, f.Name, f.Usage) - }) - return nil -} - -// For cli.RequiredFlag: - -func (f *TextMarshalerFlag) IsRequired() bool { return f.Required } - -// For cli.VisibleFlag: - -func (f *TextMarshalerFlag) IsVisible() bool { return !f.Hidden } - -// For cli.CategorizableFlag: - -func (f *TextMarshalerFlag) GetCategory() string { return f.Category } - -// For cli.DocGenerationFlag: - -func (f *TextMarshalerFlag) TakesValue() bool { return true } -func (f *TextMarshalerFlag) GetUsage() string { return f.Usage } -func (f *TextMarshalerFlag) GetEnvVars() []string { return f.EnvVars } - -func (f *TextMarshalerFlag) GetValue() string { - t, err := f.Value.MarshalText() - if err != nil { - return "(ERR: " + err.Error() + ")" - } - return string(t) -} - -func (f *TextMarshalerFlag) GetDefaultText() string { - if f.DefaultText != "" { - return f.DefaultText - } - return f.GetValue() -} - -// GlobalTextMarshaler returns the value of a TextMarshalerFlag from the global flag set. -func GlobalTextMarshaler(ctx *cli.Context, name string) TextMarshaler { - val := ctx.Generic(name) - if val == nil { - return nil - } - return val.(textMarshalerVal).v -} - var ( _ cli.Flag = (*BigFlag)(nil) _ cli.RequiredFlag = (*BigFlag)(nil) diff --git a/internal/flags/helpers.go b/internal/flags/helpers.go index 0fd37151eb..fd706869f1 100644 --- a/internal/flags/helpers.go +++ b/internal/flags/helpers.go @@ -256,9 +256,6 @@ func AutoEnvVars(flags []cli.Flag, prefix string) { case *BigFlag: flag.EnvVars = append(flag.EnvVars, envvar) - case *TextMarshalerFlag: - flag.EnvVars = append(flag.EnvVars, envvar) - case *DirectoryFlag: flag.EnvVars = append(flag.EnvVars, envvar) }