Improvements to `run` and `list` commands #230

christian-bromann · 2023-04-30T02:07:15Z

According to our internal proposal this patch makes some changes to the run and list commands. These features are behind an environment flag RUNME_EXPERIMENTAL_CLI. So the current behavior doesn't change. The new (experimental) CLI behavior works as following:

the list command now lists all commands of all markdown files within a project
- it only shows the command id and the description, everything else seems not relevant
the run command
- now only takes the block name to execute it
- can receive more block names which will be run sequentially
- now has a -p or --parallel flag to run a set of commands in parallel
- allows to execute sequential commands and parallel ones through one CLI call, e.g. runme run foo bar loo -p faz baz -s loo bar will run: foo, bar and loo sequentially, then will run faz and baz in parallel and lastly loo and bar sequential again

List Command Demo:

Screenshot of the WebdriverIO project based on webdriverio/webdriverio#10237

Run Command Demo:

Reviewer Notes:
This patch basically does the following:

moves flat functions like getCodeBlocks or readMarkdownFile into the project module
implements a getAllMarkdownFiles in the project module to find all markdown files
implement LookUpCodeBlockById to find a block just by its name
unrelated: removed -s for server address as it seems to be better suited (and more used) for providing the sequential flag

Outstanding:

read from .gitignore to not include markdown files matching these patterns (otherwise we would e.g. read from a bunch of node_modules which would be quite overwhelming
write unit tests
more thorough testing of new functionality
detect duplicate commands and throw errors
improve error handling

sourishkrout · 2023-05-02T12:50:50Z

The scope of the original ticket #229 is about parallel/multi-target changes, there wasn't green light for any other changes yet @christian-bromann.

mxsdev

Really loving the UX changes, would love to see this implemented asap!

The runner-based implementation is also reasonable, though I do have some changes in mind. But the broad-strokes idea is good!

mxsdev · 2023-05-02T17:15:34Z

internal/cmd/run.go

- ctx, cancel := ctxWithSigCancel(cmd.Context())
- defer cancel()
+ inParallelMode := false
+ blocks := os.Args[2:]


There is an args argument in the RunE function. This will parse out all cli flags.

Do you think we can then use cobra's argument parsing facilities for the parallel/serial flags, and remove manual parsing?

mxsdev · 2023-05-02T17:19:02Z

internal/cmd/run.go

+ cmd.Flags().BoolVarP(&runInParallel, "parallel", "p", false, "Run commands in parallel. (experimental)")
+ cmd.Flags().BoolVarP(&runSequential, "sequential", "s", true, "Run commands sequentially. (experimental)")
+ cmd.Flags().BoolVarP(&onlyCommandIO, "onlyCommandOutput", "", false, "If set, Runme will only output command output. (experimental)")


Cobra has StringSliceP, which would allow for the following syntax:

runme run one two three -p four,five,six -s seven,eight,nine

Do you think that would be sufficient for this purpose?

Oh yeah totally! Nice

mxsdev · 2023-05-02T17:20:28Z

internal/cmd/run.go

+ if blockID == "--experimental" {
+ continue
+ }


There are a lot more valid flags than "--experimental" which you are not catching, which will presumably be run as blocks.

For example, running runme run -s a b c tries to parse -s as a code block

mxsdev · 2023-05-02T17:22:06Z

internal/project/project.go

+func (p *Project) ReadMarkdownFile(relativePath string, args []string) ([]byte, error) {
+ absPath := path.Join(p.RootDir, relativePath)
+ rootDir := path.Dir(absPath)
+ fileName := path.Base(absPath)


Suggested change

func (p *Project) ReadMarkdownFile(relativePath string, args []string) ([]byte, error) {

absPath := path.Join(p.RootDir, relativePath)

rootDir := path.Dir(absPath)

fileName := path.Base(absPath)

func (p *Project) ReadMarkdownFile(absPath string, args []string) ([]byte, error) {

rootDir := path.Dir(absPath)

fileName := path.Base(absPath)

Why not use an absolute path? The relative option seems to make some other parts of the code a little cumbersome

mxsdev · 2023-05-02T17:25:54Z

internal/cmd/run.go

+ p := project.New(fChdir)
+ if !isInExperimentalMode() {
+ filePath := path.Join(fChdir, fFileName)
+ blocks, err := p.GetCodeBlocks(filePath[len(p.RootDir):], fAllowUnknown, fIgnoreNameless)


What is the purpose of filePath[len(p.RootDir):]?

At any rate, one of the methods in filepath would be much preferable rather than to do manual parsing...

mxsdev · 2023-05-02T18:30:01Z

internal/project/project.go

-
- ref, err := r.Head()
+func (p *Project) getAllMarkdownFiles() ([]string, error) {
+ root, err := os.Getwd()


Suggested change

root, err := os.Getwd()

root := p.RootDir

mxsdev · 2023-05-02T18:35:27Z

internal/project/project.go

- if ref.Name().IsBranch() {
- return ref.Name().Short()
+ fsys := os.DirFS(root)
+ matches, err := fs.Glob(fsys, "**/*.md")


This does not work as expected, and in particular will ignore files in the root directory.

You should probably use a WalkDir-based solution instead.

mxsdev · 2023-05-02T18:37:04Z

internal/project/project.go

- if err != nil {
- return nil, errors.New("error while querying repository branches")
+ for _, file := range files {
+ codeBlock, err := p.GetCodeBlocks(file[len(p.RootDir):], false, true)


Suggested change

codeBlock, err := p.GetCodeBlocks(file[len(p.RootDir):], false, true)

codeBlock, err := p.GetCodeBlocks(file[len(p.RootDir):], allowUnknown, ignoreNameless)

Should pass these down

internal/cmd/run.go

mxsdev · 2023-05-02T18:45:59Z

internal/cmd/run.go

+ select {
+ case <-wgDone:
+ // carry on
+ break


This break does nothing since there is no surrounding loop

Co-authored-by: Max Stoumen <[email protected]>

christian-bromann · 2023-05-18T18:26:05Z

Closing in favor of #236

christian-bromann added 4 commits April 27, 2023 16:15

enhance list command

b5b9a01

finish list command

f6812e9

enable to run sequentially

908a335

better CLI interface

78cd393

christian-bromann changed the title ~~Improved CLI~~ Improvements to run and list commands Apr 30, 2023

christian-bromann marked this pull request as draft April 30, 2023 02:08

christian-bromann requested review from sourishkrout and mxsdev April 30, 2023 03:24

christian-bromann added 12 commits April 29, 2023 20:26

revert not needed changes

a9283fe

fix linter

1bdbb00

rename experimental flag to RUNME_EXPERIMENTAL_CLI

78fbf8b

more lint fixes

c566586

remove unnecessary lines

d799043

don't require git when loading project

2c277f1

add back # of Commands

7110a58

better error handling

b4db598

read gitignore and ignore files that match patterns

9199819

performance improvements

1cc00bd

add flag to ignore code blocks without name defined

ef36bc4

allow experimental as global parameter

9f3eb99

mxsdev suggested changes May 2, 2023

View reviewed changes

Update internal/cmd/run.go

71f22bb

Co-authored-by: Max Stoumen <[email protected]>

christian-bromann closed this May 18, 2023

christian-bromann deleted the cb-better-cli branch May 18, 2023 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to `run` and `list` commands #230

Improvements to `run` and `list` commands #230

christian-bromann commented Apr 30, 2023 •

edited

Loading

sourishkrout commented May 2, 2023 •

edited

Loading

mxsdev left a comment

mxsdev May 2, 2023

mxsdev May 2, 2023

christian-bromann May 2, 2023

mxsdev May 2, 2023

mxsdev May 2, 2023

mxsdev May 2, 2023

mxsdev May 2, 2023

mxsdev May 2, 2023

mxsdev May 2, 2023

mxsdev May 2, 2023

christian-bromann commented May 18, 2023

	codeBlock, err := p.GetCodeBlocks(file[len(p.RootDir):], false, true)
	codeBlock, err := p.GetCodeBlocks(file[len(p.RootDir):], allowUnknown, ignoreNameless)

Improvements to run and list commands #230

Improvements to run and list commands #230

Conversation

christian-bromann commented Apr 30, 2023 • edited Loading

sourishkrout commented May 2, 2023 • edited Loading

mxsdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-bromann commented May 18, 2023

Improvements to `run` and `list` commands #230

Improvements to `run` and `list` commands #230

christian-bromann commented Apr 30, 2023 •

edited

Loading

sourishkrout commented May 2, 2023 •

edited

Loading