-
Notifications
You must be signed in to change notification settings - Fork 59
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 support for TOML tables to fftoml package #48
Add support for TOML tables to fftoml package #48
Conversation
Consider using a |
I'm totally open to |
I personally never use |
@peterbourgon I could have it attempt both? e.g. try |
Interesting. That could definitely work, but it feels a bit too... implicit for my taste. I think I'd rather let users set the separator rune (string?) with a default of |
Perhaps an option on the parser itself? e.g. package fftoml
func Parser(r io.Reader, set(name, value string) error) error {
return ParserWith()(r, set)
}
type config struct {
separator string
}
func FlagSeparator(s string) Option {
return func(c *config) {
c.separator = s
}
}
func ParserWith(opts ...Option) ff.ConfigFileParser {
config := config{separator: "."}
for _, opt := range opts {
opt(&config)
}
// ...
} Then you could do |
Yeah, that's essentially what I had in mind 👍 unless you can think of a nicer API. (A couple of small nits with the example code, but that's better left for review.) |
@peterbourgon 👍 I like it. I pushed up that change. |
fftoml/fftoml.go
Outdated
return ParserWith()(r, set) | ||
} | ||
|
||
type config struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather see this type as exported, so Config, to avoid having functions in the package's API that operate on unexported types
fftoml/fftoml.go
Outdated
|
||
// FlagSeparator is an option which configures a separator | ||
// to use when constructing a flag name | ||
func FlagSeparator(s string) Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think this name is too vague. Is there a name for the TOML blocking feature, the thing we're segmenting config information with? I'd prefer something like... BlockSeparator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the correct TOML term is a "table" https://github.com/toml-lang/toml#table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about TableDelimiter(string)
?
fftoml/fftoml.go
Outdated
type Option func(*config) | ||
|
||
// FlagSeparator is an option which configures a separator | ||
// to use when constructing a flag name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this comment a bit more descriptive, and add a sentence like "The default separator is a period .
."
fftoml/fftoml.go
Outdated
// By default the returned Parser uses a "." as a separator | ||
func ParserWith(opts ...Option) ff.ConfigFileParser { | ||
return func(r io.Reader, set func(name, value string) error) error { | ||
config := config{separator: "."} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what do you think about adding a Parse method to the Config struct, matching the ConfigFileParser signature, and returning that? e.g.
func ParserWith(opts ...Option) ff.ConfigFileParser {
c := Config{
separator: ".",
}
for _, opt := range opts {
opt(&c)
}
return c.Parse
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is nice. Does it continue to make sense to call it Config? Perhaps there is a more suitable name. Perhaps ConfigFileParser
to match the ff.ConfigFileParser
function signature?
Restructure call to ff.Parse Co-Authored-By: Peter Bourgon <peterbourgon@users.noreply.github.com>
… gm/support-toml-tables
@peterbourgon I made some adjustments based on your comments :) What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two very minor things and then I'm happy to merge. Thanks a bunch!
fftoml/fftoml.go
Outdated
// used to prefix table names onto keys when constructing | ||
// their associated flag name. | ||
// The default delimeter is "." | ||
func TableDelimeter(d string) Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Just a minor nit for consistency :)
func TableDelimeter(d string) Option { | |
func WithTableDelimeter(d string) Option { |
fftoml/fftoml.go
Outdated
// TableDelimeter is an option which configures a delimeter | ||
// used to prefix table names onto keys when constructing | ||
// their associated flag name. | ||
// The default delimeter is "." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The default delimeter is "." | |
// The default delimeter is "." | |
// | |
// For example, given the following TOML | |
// | |
// [section.subsection] | |
// value = 10 | |
// | |
// Parse will match to a flag with the name `-section.subsection.value` by default. | |
// If the delimiter is "-", Parse will match to `-section-subsection-value` instead. |
Oh, oops — it's spelled |
Ahh good catch 🤦♂. Cheers @peterbourgon ! |
This adds support for TOML tables. It does so by concatenating nested keys with the
-
character.For example:
Will set the flag
-a-b
with the value"c"
.This swaps to a different toml parser (namely
github.com/pelletier/go-toml
), however, upon reflection the BurntSushi version is probably still useable. I can swap to attempt that if that would be more preferrable.