Skip to content
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

Refactor flags, defaults and usage #2530

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

AnkushinDaniil
Copy link
Contributor

This pull request includes several changes to the cmd/juno package, primarily focused on refactoring the code to use a new setting struct for flag and default value management.

Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 94.87179% with 4 lines in your changes missing coverage. Please review.

Project coverage is 73.52%. Comparing base (ba3fcff) to head (10311ae).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/juno/dbcmd.go 71.42% 1 Missing and 1 partial ⚠️
cmd/juno/juno.go 97.18% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2530      +/-   ##
==========================================
- Coverage   73.62%   73.52%   -0.11%     
==========================================
  Files         136      136              
  Lines       16774    16662     -112     
==========================================
- Hits        12350    12250     -100     
+ Misses       3555     3542      -13     
- Partials      869      870       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@weiihann
Copy link
Contributor

From my understanding, this PR seems to be "cleaner" in the sense of eliminating repetitive flag name/usage string declarations. However, the generic usage seems to complicate things further.

I would suggest the following registry and group pattern instead:

type Flag struct {
    Name         string
    DefaultValue any
    Usage        string
    Required     bool
}

type FlagGroup struct {
    Name  string
    Flags []Flag
}

type Registry struct {
    groups     map[string]FlagGroup
    defaults   map[string]any
    validators map[string]func(any) error
}

type ConfigHelper struct {
    registry *Registry
    cmd      *cobra.Command
}

func (c *ConfigHelper) ConfigureCommand(cmd *cobra.Command) error {
    c.cmd = cmd
    
    for _, group := range c.registry.groups {
        for _, flag := range group.Flags {
            switch v := flag.DefaultValue.(type) {
            case string:
                cmd.Flags().String(flag.Name, v, flag.Usage)
            // ... handle other types
            }
            
            if flag.Required {
                cmd.MarkFlagRequired(flag.Name)
            }
        }
    }
    
    return nil
}

// Define flag groups for better organization
var (
    NetworkFlags = FlagGroup{
        Name: "Network Configuration",
        Flags: []Flag{
            {
                Name:         "network",
                DefaultValue: utils.Mainnet,
                Usage:        "Options: mainnet, sepolia, sepolia-integration",
            },
            {
                Name:         "cn-name",
                DefaultValue: "",
                Usage:        "Custom network name",
            },
            // ... other network related flags
        },
    }
    ... // more groups
)

func NewCmd(config *node.Config, run func(*cobra.Command, []string) error) *cobra.Command {
    registry := flags.NewRegistry()
    
    // Register all flag groups
    registry.RegisterGroup(flags.NetworkFlags)
    registry.RegisterGroup(flags.HTTPFlags)
    // ... register other groups
    
    // Add flag validators
    registry.AddValidator("network", validateNetwork)
    // ... add other validators
    
    ...add other things 
}

To complete this is much more work, but grouping flags keep it organized and we can expand our CLI tools easily.

@AnkushinDaniil AnkushinDaniil marked this pull request as draft February 20, 2025 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants