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

Add PathFlag #670

Merged
merged 1 commit into from
Sep 28, 2017
Merged

Add PathFlag #670

merged 1 commit into from
Sep 28, 2017

Conversation

VMitov
Copy link
Contributor

@VMitov VMitov commented Sep 26, 2017

No description provided.

@VMitov VMitov mentioned this pull request Sep 26, 2017
@VMitov
Copy link
Contributor Author

VMitov commented Sep 26, 2017

This need additional work that is discussed in #666.

}

// ApplyWithError populates the flag given the flag set and environment
func (f *PathFlag) ApplyWithError(set *flag.FlagSet) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it we don't need to resolve the path in here since it'll be relative to the working directory.

altsrc/flag.go Outdated
@@ -231,3 +255,19 @@ func isEnvVarSet(envVars []string) bool {
func float64ToString(f float64) string {
return fmt.Sprintf("%v", f)
}

func stringToPath(context *cli.Context, path string) (string, error) {
inputSourcePath := context.String("load")
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, yes, it'll be difficult to access the path of the loaded file from here.

One approach could be to add another method to InputSourceContext that returns the filepath of the source; this value would then be passed in by the YAML and TOML loaders to the constructed MapInputSource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was thinking in the same direction. I'll do that. Cheers!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@VMitov
Copy link
Contributor Author

VMitov commented Sep 27, 2017

FYI I'll rebase and squash the commits before merging.

@@ -9,6 +9,8 @@ import (
// InputSourceContext is an interface used to allow
// other input sources to be implemented as needed.
type InputSourceContext interface {
Source() string
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment here to make it more clear for implementers what this should return?

@jszwedko
Copy link
Contributor

This looks great @VMitov ! Thank you for adding the comprehensive tests. I just left one note about commenting the interface, but I think this is good to go after that.

@jszwedko
Copy link
Contributor

👍 looks great! Let me know if you still want to squash.

@VMitov
Copy link
Contributor Author

VMitov commented Sep 28, 2017

Rebased.

Copy link
Contributor

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Thanks again!

@jszwedko jszwedko merged commit b2bf3c5 into urfave:v2 Sep 28, 2017
@VMitov VMitov deleted the path_flag branch September 29, 2017 08:36
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.

None yet

2 participants