chore(modular-cmds): flags helper, docs#700
Conversation
|
ajaskolski
force-pushed
the
introduce-shared-flags-modular-cmds
branch
from
1c130fd to
6231027
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a reusable flags helper package and enhances documentation for the state commands in the CLI framework. The changes support the CLD-1155 initiative by standardizing flag definitions and improving user-facing command documentation.
Changes:
- Created a new
flagspackage with helpers for common CLI flags (environment,print,output) - Updated state commands to use the new flags helpers with shorter flag names and deprecated aliases for backward compatibility
- Enhanced command documentation with detailed descriptions and examples using the
cli.LongDescandcli.Examplesutilities
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| engine/cld/commands/flags/flags.go | New package with reusable flag helpers for environment, print, and output flags |
| engine/cld/commands/flags/flags_test.go | Comprehensive tests for the new flags package |
| engine/cld/commands/state/generate.go | Refactored to use flags helpers, added documentation, and introduced --print flag to control output behavior |
| engine/cld/commands/state/command.go | Added validation method and documentation strings for the state command |
| engine/cld/commands/state/command_test.go | Updated tests to verify new flag behavior and removed redundant tests |
| engine/cld/legacy/cli/commands/state_test.go | Updated tests to reflect that environment flag is now local instead of persistent |
| engine/cld/commands/commands_test.go | Updated tests to verify environment flag is local to subcommands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ajaskolski
force-pushed
the
introduce-shared-flags-modular-cmds
branch
from
a655c4f to
737e480
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Set a timeout for the view operation as it may take a while | ||
| envdir := cfg.Domain.EnvDir(f.environment) | ||
| viewTimeout := 10 * time.Minute |
There was a problem hiding this comment.
How does CCIP use this command if their state generation takes so long? Perhaps locally it's much faster and occurs in under 10 minutes?
There was a problem hiding this comment.
really depends how many things you have onchain
There was a problem hiding this comment.
it could be ccip dont use the same context when in their logic.
| ) | ||
|
|
||
| // generateFlags holds all flags for the generate command. | ||
| type generateFlags struct { |
There was a problem hiding this comment.
@graham-chainlink What do you think of this convention?
Should we prefer GetX commands in the implementation, or use the persistent flags?
There was a problem hiding this comment.
i kind of like it, by retrieving all the flag values in the newGenerateCmd function, and then pass in the generateFlags into runGenerate means that runGenerate does not need to know how to read the flag, easier testing and cleaner separation
There was a problem hiding this comment.
downside is if we have a lot of nested child commands , then we have to keep passing the flags down, but i dont think we have this problem here.
There was a problem hiding this comment.
The struct is fine. I was more referring to the intiialization of values in the fields.
I think it would be better to initialize the struct with
g := generateFlags{
X: cmd.GetString("")
}
Note this is not exact code. I'm not sure of the exact methods
There was a problem hiding this comment.
i see, not strong opinion, i like the pointer approach eg StringVarP, in this case, it requires less line of code as we declared and set value of the flag in the same line, so we dont have to explicitly retrieve it again.
ajaskolski
force-pushed
the
introduce-shared-flags-modular-cmds
branch
from
83cec5f to
d1bb809
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return fmt.Errorf("failed to load environment: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
in the orignal previousState flag was never used actually in the code so providing it just did nothing, added the logic for it but we may also just delete it if not used
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I finished with batch updates, if you will have moment please revisit the PR. |
ajaskolski
force-pushed
the
introduce-shared-flags-modular-cmds
branch
from
e67ca0a to
e35c4c9
Compare
|
3 participants
https://smartcontract-it.atlassian.net/browse/CLD-1155