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

Feat: Convert filepath in Flag to string slice #1601

Merged
merged 2 commits into from
Nov 29, 2022

Conversation

dearchap
Copy link
Contributor

What type of PR is this?

(REQUIRED)

  • feature

What this PR does / why we need it:

(REQUIRED)

Which issue(s) this PR fixes:

(REQUIRED)

Fixes #1166

Special notes for your reviewer:

(fill-in or delete this section)

Testing

(fill-in or delete this section)

make test

Release Notes

(REQUIRED)


@dearchap dearchap requested a review from a team as a code owner November 28, 2022 02:04
Usage string // usage string for help output
Category string // category of the flag, if any
DefaultText string // default text of the flag for usage purposes
FilePath []string // file paths to load value from
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be named to FilePaths then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats what I initially was about to do but thought to keep the name same since behavior is same. I can change it. @meatballhat any thoughts ?

Copy link
Contributor

Choose a reason for hiding this comment

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

With many file paths, what will happen if first path does not exist? Will value from the second path overwrite the first?

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of a rename to FilePaths 👍🏼 and to @abitrolly's point it'd be nice to have clearer documentation around the expected behavior 🙇🏼 (in a later PR!)

@abitrolly
Copy link
Contributor

Not clear how it is supposed to be used. Link to relevant usage examples could help that.

@dearchap
Copy link
Contributor Author

@abitrolly https://cli.urfave.org/v2/examples/flags/#values-from-files . Previously multiple files would be specified by doing

FilePath: "/path/file1, /path/file2, ....'

So this change makes the file paths more explicit

FilePath: []string{"/path/file1","/path/file2"....}

@abitrolly
Copy link
Contributor

@dearchap should the example be updated then?

            &cli.StringFlag{
                Name:     "password",
                Aliases:  []string{"p"},
                Usage:    "password for the mysql database",
                FilePath: "/etc/mysql/password",
            },

@dearchap
Copy link
Contributor Author

@abitrolly Yes. Maybe we'll do a separate PR to update the docs since this change is going into v3 ?

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.

Oh! You already renamed to FilePaths 🤦🏼 Thank you!

@dearchap dearchap merged commit 02495e3 into urfave:main Nov 29, 2022
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.

Convert FilePath field in flags from string to string array
3 participants