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

"existingfile"/"existingdir" always checks default value #26

Closed
alecthomas opened this issue Nov 17, 2018 · 10 comments · Fixed by #297
Closed

"existingfile"/"existingdir" always checks default value #26

alecthomas opened this issue Nov 17, 2018 · 10 comments · Fixed by #297

Comments

@alecthomas
Copy link
Owner

When type:"existingfile" is given, kong checks if the default value exists as a file even the config value is explicitly set to something else.

@nsemikov
Copy link
Contributor

nsemikov commented Aug 4, 2020

+1

@alecthomas alecthomas changed the title "existingfile" always checks default value "existingfile"/"existingdir" always checks default value Dec 1, 2021
@pyqlsa
Copy link
Contributor

pyqlsa commented Jan 8, 2022

+1 Also seeing this.

@Grazfather
Copy link

Just hit this.

@pyqlsa
Copy link
Contributor

pyqlsa commented May 1, 2022

#297 is intended to address this, although it admittedly feels like a bit of a hack.

Thought process: applied updates inside individual mappers because these mappers feel like they are intended to check more than just the 'datatype'. If this fix were to be applied outside of the individual mappers (i.e. something like "check if has default, check if value passed on cli, de-conflict, then apply resolver"), the updated behavior would affect all resolvers, not just the ones in question. Also, in general, it feels appropriate for the rest of the resolvers to continue performing 'datatype' checks on both the default values and passed values whenever they occur.

There's a chance I'm missing some context around resolvers and their order of operations, so I'm definitely open to feedback on the PR.

Edit: this hack will mask #190 if the default doesn't exist and a value is passed on the cli.

@MrMarble
Copy link

MrMarble commented May 3, 2022

This also happens without default value. If you configure it as optional, is tested anyways

@pyqlsa
Copy link
Contributor

pyqlsa commented May 16, 2022

This also happens without default value. If you configure it as optional, is tested anyways

By setting a default value, this in effect sets the value (even if the user doesn't explicitly set it). Configuring this as optional allows for the arg/flag to be unset, so making this optional would (by design) have no affect on the validation success/failure.

@MrMarble if neither a default nor a user-provided value is set, what exactly is being tested? I don't believe I've seen this behavior as you describe. If you could share an example, that would be helpful.

@MrMarble
Copy link

This also happens without default value. If you configure it as optional, is tested anyways

By setting a default value, this in effect sets the value (even if the user doesn't explicitly set it). Configuring this as optional allows for the arg/flag to be unset, so making this optional would (by design) have no affect on the validation success/failure.

@MrMarble if neither a default nor a user-provided value is set, what exactly is being tested? I don't believe I've seen this behavior as you describe. If you could share an example, that would be helpful.

If you add the type:"existingfile" to a positional argument that has optional:"" it will fail with an error saying that the file "could not be found" when the argument is not used

@pyqlsa
Copy link
Contributor

pyqlsa commented May 30, 2022

If you add the type:"existingfile" to a positional argument that has optional:"" it will fail with an error saying that the file "could not be found" when the argument is not used

@MrMarble if I'm understanding correctly, this code snippet should test your described use case. As of the updates from #297, this should work as expected.

	type CLI struct {
		File string `arg:"" optional:"" type:"existingfile"`
	}
	var cli CLI
	// no-arg cli should still pass because it's optional
	p := mustNew(t, &cli)
	_, err := p.Parse([]string{})
	require.NoError(t, err)
	require.Empty(t, cli.File)

@NiceGuyIT
Copy link

This fails when using commands. When type="existingfile" is under one command, all other commands fail. I would expect it to affect only that command.

Here's an albeit verbose example.

package main

import (
	"fmt"
	"github.com/alecthomas/kong"
)

var CLI struct {
	// Manage command
	Manage struct {
		Data      string `name:"data" group:"manage" default:"data" help:"Data directory for the files."`
		LoadFile1 string `name:"load-file1" group:"manage" required:"" type:"existingfile" default:"${data}/file1.txt" placeholder:"${data}/file1.txt" help:"Required file with existingfile type, has default."`
		LoadFile2 string `name:"load-file2" group:"manage" optional:"" type:"existingfile" placeholder:"${data}/file2.txt" help:"Optional file with existingfile type, no default."`
		LoadFile3 string `name:"load-file3" group:"manage" required:"" default:"${data}/file3.txt" placeholder:"${data}/file3.txt" help:"Required file without type, has default."`
		LoadFile4 string `name:"load-file4" group:"manage" required:"" type:"existingfile" placeholder:"${data}/file4.txt" help:"Required file with existingfile type, no default."`
	} `cmd:"" group:"manage" help:"Manage functions."`

	// Install command
	Install struct {
		Script string `name:"script" arg:"" help:"Script to install."`
		File1  string `name:"file" arg:"" help:"Existing file to install."`
	} `cmd:"" help:"Install functions."`

	// Download command
	Download struct {
		OS   string `name:"os" optional:"" default:"" help:"OS to download."`
		Arch string `name:"arch" optional:"" default:"" help:"Architecture to download."`
	} `cmd:"" help:"Download functions."`

	// Version
	Version struct {
	} `cmd:"" group:"version" name:"version" help:"Version information"`

	// License
	License struct {
	} `cmd:"" group:"license" name:"license" help:"License information"`
}

func main() {

	ctx := kong.Parse(&CLI,
		kong.Vars{
			"data": "data",
		},
	)
	switch ctx.Command() {
	case "manage":
		fmt.Println("Running manage command")

	case "install":
		fmt.Println("Running install command")

	case "download":
		fmt.Println("Running download command")

	case "version":
		fmt.Println("Running version command")

	case "license":
		fmt.Println("Running license command")

	default:
		_ = kong.DefaultHelpPrinter(kong.HelpOptions{}, ctx)
	}

}

kong-example --help works.

$ ./kong-example --help
Usage: kong-example <command>

Flags:
  -h, --help    Show context-sensitive help.

Commands:
  install <script> <file>
    Install functions.

  download
    Download functions.

manage
  manage --load-file1=${data}/file1.txt --load-file3=${data}/file3.txt --load-file4=${data}/file4.txt
    Manage functions.

version
  version
    Version information

license
  license
    License information

Run "kong-example <command> --help" for more information on a command.

All other commands fail.

$ ./kong-example install
kong-example: error: --load-file1: stat /home/user/Projects/kong-example/data/file1.txt: no such file or directory

$ ./kong-example download
kong-example: error: --load-file1: stat /home/user/Projects/kong-example/data/file1.txt: no such file or directory

$ ./kong-example version
kong-example: error: --load-file1: stat /home/user/Projects/kong-example/data/file1.txt: no such file or directory

$ ./kong-example license
kong-example: error: --load-file1: stat /home/user/Projects/kong-example/data/file1.txt: no such file or directory

@pyqlsa
Copy link
Contributor

pyqlsa commented Jul 17, 2022

@NiceGuyIT good find! I think #319 should fix this.

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 a pull request may close this issue.

6 participants