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

feature request: min length, max length, and whitespace trimming #125

Closed
insasho opened this issue Apr 30, 2016 · 3 comments
Closed

feature request: min length, max length, and whitespace trimming #125

insasho opened this issue Apr 30, 2016 · 3 comments

Comments

@insasho
Copy link

insasho commented Apr 30, 2016

The .String() type is versatile, but often it is more permissive than we want. For example, some flags might have a maximum or minimum length. Minimum length is not naturally expressed with .Required() as that implies a minimum length of 1 and does not trim whitespace.

Suggestion: Add some modifiers to FlagClause such as:
a. MinLength to enforce a min length
b. MaxLength to enforce a max length
c. Trimmed to always whitespace-trim the flag before processing

Using custom flag.Value types doesn't appear to be an elegant solution to this problem because:

  1. they are awkward to chain, leading to a proliferation of flag-specific flag.Value implementations
  2. errors returned by .Set are not displayed by kingpin with the flag name prefixed, which makes implementation of useful error messages awkward. For example, a return fmt.Errorf("max length of %d exceeded", m.maxLength) from a flag.Value .Set() prints
progname: error: max length of 24 exceeded, try --help

It would be nice if this also contained the name of the flag that was being validated.

If I misunderstood how Kingpin is to be used, please send me in the right direction. I see v1.3.4 added "Add post-app and post-cmd validation hooks. This allows arbitrary validation to be added." but I couldn't find any examples or additional docs on this.

Thanks!

@insasho
Copy link
Author

insasho commented Apr 30, 2016

After additional investigation, it looks like .Action(), .PreAction(), and .Validate() methods are also inappropriate for applying custom validation to user-provided strings because the actual string value is awkward to extract from the data structures made available to those methods. The Validate API allows identifying a FlagClause by name, though the Action API requires searching over c.Elements manually. In both cases, retrieving the actual value is impossible because .value is not exported and there is no getter. Am I missing something?

Thanks!

@alecthomas alecthomas mentioned this issue May 5, 2016
28 tasks
@alecthomas
Copy link
Owner

Hi there. Yes, the actions and Validate methods were poorly thought out. Fixing that is the first item in the v3 wish list (#90).

I don't think adding functions for specific types of validation is the right approach though, but fixing the Validate functions is.

I've added a link to this issue for details.

Thanks for the report.

@ches
Copy link

ches commented Aug 5, 2016

the actual string value is awkward to extract from the data structures made available to those methods. The Validate API allows identifying a FlagClause by name, though the Action API requires searching over c.Elements manually. In both cases, retrieving the actual value is impossible because .value is not exported and there is no getter.

I've had the same thoughts when trying to figure out how to use these, I'm glad it's not just me 😄 For instance, where does the error that my Action returns go/how is it handled, and how can I actually get at values with Validate? There were no unit tests or examples added for Validate to shed light.

I'll look forward to v3 for these 😄

jetzerb added a commit to jetzerb/usql that referenced this issue Jan 16, 2020
The code that handled pset-based commandline arguments using
`kingpin`'s `PreAction` callback was not working as intended.
The context variable passed to the callback contains
information on _all_ commandline arguments, and [doesn't
provide any information specific to the particular flag being
processed](alecthomas/kingpin#125 (comment)).

There was also a problem with the callback construction because of
[the way go implements closures](https://golang.org/doc/faq#closures_and_goroutines).
The result is that the variables reflect the last value from the `for`
loop, which is why, as pointed out in xo#97, there is no difference
between the `-J` and `-C` options: the last iteration of the `for` loop
was for the CSV entry, so regardless which of the `-A, -F, -H, -R, -t,
-T, -x , -z, -0, -J, or -C` options are specified, they all end up
setting `format=csv`.

A new custom type `pset` was created to handle the string-based flags,
and the existing for loop has been modified to properly handle the
boolean flags.
niurkacanals added a commit to niurkacanals/Database-developer-for-sql that referenced this issue Sep 2, 2022
The code that handled pset-based commandline arguments using
`kingpin`'s `PreAction` callback was not working as intended.
The context variable passed to the callback contains
information on _all_ commandline arguments, and [doesn't
provide any information specific to the particular flag being
processed](alecthomas/kingpin#125 (comment)).

There was also a problem with the callback construction because of
[the way go implements closures](https://golang.org/doc/faq#closures_and_goroutines).
The result is that the variables reflect the last value from the `for`
loop, which is why, as pointed out in #97, there is no difference
between the `-J` and `-C` options: the last iteration of the `for` loop
was for the CSV entry, so regardless which of the `-A, -F, -H, -R, -t,
-T, -x , -z, -0, -J, or -C` options are specified, they all end up
setting `format=csv`.

A new custom type `pset` was created to handle the string-based flags,
and the existing for loop has been modified to properly handle the
boolean flags.
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

No branches or pull requests

3 participants