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: Add support for validation functions #1785

Merged
merged 12 commits into from
Jun 27, 2023
Merged

Conversation

dearchap
Copy link
Contributor

@dearchap dearchap commented Jun 26, 2023

What type of PR is this?

(REQUIRED)

  • feature

What this PR does / why we need it:

(REQUIRED)

This PR adds a framework for allowing users to be able to define custom validators on flags. Some very basic rudimentary ones have been provided for demonstration purposes to be able to gather user feedback on easy of use. Other validation functions may be provided in the future if users requests common ones. Note that the validation happens in the same context as when the value is set from the command line so slice flag validations for length may not work as expected.

Which issue(s) this PR fixes:

(REQUIRED)

Special notes for your reviewer:

(fill-in or delete this section)

Testing

(fill-in or delete this section)

Add new validation test cases,

Release Notes

(REQUIRED)


dearchap added 2 commits June 26, 2023 21:33
Add new fv

Fix more tests

Add validator

Add sample validation functions

Revert some changes
@dearchap dearchap marked this pull request as ready for review June 27, 2023 00:42
@dearchap dearchap requested a review from a team as a code owner June 27, 2023 00:42
Copy link
Member

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

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

  • I really like this capability
  • I'm super nervous about depending on golang.org/x/exp
  • I don't like the alternatives to depending on golang.org/x/exp
  • I'm nervous about how much this expands the API surface area

What do you think about making some/most of this an "addon" library like with urfave/cli-docs and urfave/cli-altsrc?

command_test.go Outdated Show resolved Hide resolved
flag_impl.go Outdated Show resolved Hide resolved
flag_impl.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
flag_impl.go Outdated Show resolved Hide resolved
flag_validation.go Outdated Show resolved Hide resolved
flag_validation.go Outdated Show resolved Hide resolved
@dearchap dearchap temporarily deployed to frogbot June 27, 2023 09:02 — with GitHub Actions Inactive
@dearchap dearchap added area/v1 relates to / is being considered for v1 area/v3 relates to / is being considered for v3 and removed area/v1 relates to / is being considered for v1 labels Jun 27, 2023
@github-actions
Copy link

What is Frogbot?

Copy link
Member

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

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

Aww, I liked the validation helper bits! 😭 WDYT about renaming urfave/cli-altsrc to urfave/cli-ext and including them over there?

flag_impl.go Outdated Show resolved Hide resolved
flag_validation_test.go Show resolved Hide resolved
flag_validation_test.go Show resolved Hide resolved
@dearchap dearchap temporarily deployed to frogbot June 27, 2023 15:22 — with GitHub Actions Inactive
@dearchap
Copy link
Contributor Author

Aww, I liked the validation helper bits! 😭 WDYT about renaming urfave/cli-altsrc to urfave/cli-ext and including them over there?

Hmm thats not a bad idea at all. I'm ok with that. cli-docs would go in there as well ? Would you like to take point on that ?

@github-actions
Copy link


  • Frogbot also supports Contextual Analysis, Infrastructure as Code Scanning and Secrets Detection. These features are included as part of the JFrog Advanced Security package, which isn't enabled on your system.

@dearchap
Copy link
Contributor Author

Aww, I liked the validation helper bits! 😭 WDYT about renaming urfave/cli-altsrc to urfave/cli-ext and including them over there?

Hmm thats not a bad idea at all. I'm ok with that. cli-docs would go in there as well ? Would you like to take point on that ?

@dearchap dearchap merged commit fc3b515 into urfave:main Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v3 relates to / is being considered for v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants