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

Added StringArray Function #181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

pjdufour
Copy link

@pjdufour pjdufour commented Sep 15, 2018

This PR adds a StringArray() []string function to each value. I will make a PR to viper after this is merged, so that the StringArray variables use this new function. This is needed since binding to pflag prevents the use of values that can't be packed/unpacked using CSV. With this function, viper can pull array values directly from pflag maintaining the original values.

@CLAassistant
Copy link

CLAassistant commented Sep 15, 2018

CLA assistant check
All committers have signed the CLA.

@eparis
Copy link
Collaborator

eparis commented Sep 15, 2018

You added a new function to every defined type which appears to only have meaning on StringArray and StringSlice. Why? If it isn't part of the Value interface no one can come to rely on it generically, so they would have had to cast to a specific pflag type. And if they cast to a specific pflag type why would they bother to use StringArray() for any other types?

My second question is, what type of string can't be packed/unpacked with a CSV? Can you write some test that shows the problem?

@pjdufour
Copy link
Author

pjdufour commented Sep 15, 2018

The specific CLI command using cobra/viper/pflag that I needed to run was:

go run main.go serve -d '{name:dc_amenities, uri: `"data/" + @z + "-" + @x + "-" + @y + "/dc_amenities.geojsonl"`}'

It works with this PR. I first tried a quick hack in viper using reflection to handle this case; however, it was not feasible without refractoring. You can't type cast to access fields from un-exported structs. All the values are un-exported types. I'm happy to consider other potential ways.

@pjdufour
Copy link
Author

Are there any outstanding questions that I can help answer? Thanks for consideration.

@therealmitchconnors
Copy link
Collaborator

@pjdufour is this change still of interest to you? I am bumping up against similar limitations, and I'm considering creating something like a SliceValue and ArrayValue interface to export list-specific functions for Viper to consume. Interested in collaborating?

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.

4 participants