Closed mark-adams closed 8 years ago
@kelseyhightower Hope you had a great holiday! Do you have any feedback on this PR? I'd love to get started on changes if you have any. :smile:
Hi everyone,
Any chance I could get a review and merge on this PR? Thanks!
:+1:
I think we can do without a fieldParser
type, and without separating Parse
from Set
.
Parse
and Set
are already called together in every case except inside the slices. In that case however, a function that did both the parse and set (and thus took a string
and *reflect.Value
as source and destination arguments) could still work. So could we have a function like that for each atomic type (maybe promote all ints to int64 and float32s to float64), and the slice handling calls into the appropriate one?
Final nit: the type codes are wrong in the tests' Errorf
calls (they aren't string
and int
, they are []string
and []int
). %q
should work for all of them.
EDIT: alternatively, the switch could go inside a function that handles all types instead of having distinct functions for each atomic type.
@mark-adams can you rebase this? @teepark Can you make those changes after we merge this after the rebase?
sounds good to me.
LGTM
@teepark Feel free to submit a follow up PR to clean things up.
Fixes #31 by adding support for populating slices from comma separated environment variables.
To support this, I split out the switch statement from
Process()
intoParse()
andSet()
functions which deal with deserializing values from a specific type and setting the values using reflection (respectively). This was done primarily to allow for reuse of the existing string conversion logic for each type with slices containing those types.Tests included