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

Flag value and command valid arguments functions support. #27

Merged
merged 23 commits into from
Oct 10, 2023

Conversation

akemrir
Copy link
Contributor

@akemrir akemrir commented Sep 1, 2023

Flag value and command valid arguments functions support.

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2023

Codecov Report

Merging #27 (9ad648f) into main (9f53ab1) will decrease coverage by 1.71%.
Report is 1 commits behind head on main.
The diff coverage is 78.42%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
- Coverage   86.81%   85.11%   -1.71%     
==========================================
  Files          11       13       +2     
  Lines        2094     2317     +223     
==========================================
+ Hits         1818     1972     +154     
- Misses        194      252      +58     
- Partials       82       93      +11     
Files Coverage Δ
comp_arguments.go 100.00% <100.00%> (ø)
flag.go 86.75% <100.00%> (ø)
option.go 100.00% <ø> (ø)
string_utils.go 100.00% <100.00%> (ø)
cli.go 80.26% <96.29%> (+1.03%) ⬆️
slice_utils.go 68.42% <68.42%> (ø)
completion.go 78.25% <75.44%> (-9.77%) ⬇️

@akemrir akemrir marked this pull request as ready for review September 6, 2023 05:50
Comment on lines 7 to 16
/*
Arg completion tag syntax (WIP):

`comp:"nofile"`
`comp:"enums=a,b,c,d;"`
`comp:"fn=funcName"`
`comp:"listExt=json,yaml,toml"`
`comp:"listFiles"` // and listDirs, listDirsAndFiles, etc.

*/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With more looking into this feature, I am thinking that adding a new tag is not the best solution, maybe simply use a parsing option is a simpler and better choice. e.g.

WithArgCompFuncs(funcs map[string]ArgCompletionFunc) ParseOpt

// where key of funcs is a flag name "-a", or "--b", or an argument name "arg1"

Just a thought, how do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you prepare example?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.

func main() {
	var args struct {
		Name string `cli:"-n, --name, Who do you want to say to" default:"tom"`

		// This argument is required.
		Text string `cli:"#R, text, The 'message' you want to send"`
	}
	mcli.Parse(&args,
		mcli.WithArgCompFuncs(map[string]mcli.ArgCompletionFunc{
			"-n": func(ctx mcli.ArgCompletionContext) []mcli.CompletionItem {
				return []mcli.CompletionItem{
					{"Tom", "Tom Smith"},
					{"John", "John White"},
				}
			},
			"text": func(ctx mcli.ArgCompletionContext) []mcli.CompletionItem {
				// biz logic
				// ...
				return []mcli.CompletionItem{
					// ...
				}
			},
		}))
	fmt.Printf("Say to %s: %s\n", args.Name, args.Text)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, cleanup done
looks actually better

)

// ArgCompletionFunc is a function to do completion for flag value or positional argument.
type ArgCompletionFunc func(ctx ArgCompletionContext) ([][]string, ShellCompDirective)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A precisely defined return type could be easier to use for library users, e.g.

// returns []CompletionItem

type CompletionItem struct {
    Value string
    Description string
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sure

Comment on lines 98 to 103
// WithCommandCompFuncs for holding completion functions for use with args completions
func WithCommandCompFunc(function ArgCompletionFunc) CmdOpt {
return CmdOpt{f: func(options *cmdOptions) {
options.argCompFunc = function
}}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option seems unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? It's separate for command.

	app.Add("s0", serve, "Serve with port and dir", 
		mcli.WithCommandCompFunc(configureFunc(config, cmdTestArguments))
	)

I have decorator to pass extra arguments, but the purpose is the same. Separate handling for commands.
It's simpler then to handle and get the function.

For flags we have slice of functions, and we need to filter out them.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understand it right, is it to complete a positional argument? If yes, it can be unified by using

WithArgCompFuncs(map[string]ArgCompletionFunc{
    "arg1": fn1,
    "arg2": fn2,
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is to complete positional argument.
You are thinking about unification of arguments passing?

Right now I am passing one function to one command.

Please suggest place to which we would pass this map.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the example text above #27 (comment), I passed the map to Parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the goal is to pass both command positional argument completion and flag completion in same place?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this unified API is simpler and more straitforward to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but we would get completion after command parsing that way?
I changed the code, but it's not evaluating properly.

Were we would pass it otherwise?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, complete positional argument after parsing flags will make it more flex and powerful, i.e. programmer can make use of parsed flags to decide which arguments to suggest.
e.g. for the above example program say, if name is "Tom", we suggest "a", "b" as text, if name is "John", we suggest "d", "e", "f".
I will take a look into the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do so. I am not pushing my current changes.

@jxskiss
Copy link
Owner

jxskiss commented Sep 12, 2023

And another question, do we need to support context-aware completion? I mean can user implement the completion functions according to the flags and/or positional arguments already provided in comand line.

@akemrir
Copy link
Contributor Author

akemrir commented Sep 13, 2023

Yes, he can.

func cmdTestArguments(config Config, ctx mcli.ArgCompletionContext) []mcli.CompletionItem {
	// fmt.Printf("%s, %#v\n", config, ctx.Args())

With ctx we pass context, user can get arguments from it.
So he can also adjust logic according to current state.

@akemrir
Copy link
Contributor Author

akemrir commented Sep 13, 2023

I had also started cleanup. I will do final PR with whole cleanup after all remarks.

akemrir and others added 2 commits September 16, 2023 18:24
Change-Id: I303548507a8cde2888a6205f6afe269423b523d9
@jxskiss
Copy link
Owner

jxskiss commented Oct 9, 2023

Finally I found some time to dive into this code, I send a PR to your branch, please check it, feedback is welcome.
akemrir#1

jxskiss and others added 4 commits October 9, 2023 19:53
Change-Id: Id76572f4712e8b32994ceecfc58e6f3fa117f7b0
Change-Id: I53bc2d0b32a95a24b50a3722291b5853c201a311
PR-003 make arg completion more flexible
@akemrir
Copy link
Contributor Author

akemrir commented Oct 10, 2023

ok, I had merged the changes from your PR to this PR
I've made also few accomodations for zsh/fish shells.
Could you take a look?

@jxskiss jxskiss merged commit 4f892a7 into jxskiss:main Oct 10, 2023
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.

3 participants