Skip to content

Commit

Permalink
internal/flags: remove low-use type TextMarshalerFlag (#30707)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
karalabe authored Oct 31, 2024
1 parent 20bf543 commit a1d049c
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 122 deletions.
11 changes: 6 additions & 5 deletions cmd/utils/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
Expand Down
114 changes: 0 additions & 114 deletions internal/flags/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package flags

import (
"encoding"
"errors"
"flag"
"fmt"
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 0 additions & 3 deletions internal/flags/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down

0 comments on commit a1d049c

Please sign in to comment.