peterbourgon / ff

Flags-first package for configuration
Apache License 2.0
1.34k stars 59 forks source link

feat: ffjson/ffyaml nested config #108

Closed jolheiser closed 1 year ago

jolheiser commented 1 year ago

Similar to the TOML parser, this allows for the JSON/YAML parser to interpret nested objects/nodes.

e.g.

{
  "myapp": {
    "foo": "bar"
  }
}
myapp:
  foo: bar

Using similar defaults from the TOML parser, the above would resolve from a flag like -myapp.foo bar

A large chunk of the TOML parser (and subsequent test) was copied and modified to work with the JSON/YAML library.

Resolves #107

peterbourgon commented 1 year ago

I'm down with considering this patch, if comparable additions are made to ffjson.

jolheiser commented 1 year ago

There are still a few things I may need some feedback on.

First, table isn't the same naming in JSON/YAML as with TOML, so I named them accordingly (as best I can tell) by using object for JSON and node for YAML.
However, this makes the naming inconsistent, so my question is for the following options:

  1. Leave them as-is
  2. Use the same name as TOML for consistency (e.g. WithTableDelimiter)
  3. Use a generic name for consistency (e.g. WithDelimiter)
    • If this option is chosen, perhaps WithTableDelimiter could be deprecated similarly to how ioutil is handled in the stdlib

Second, considering JSON is within the ff package directly, the names are a little more verbose. I don't think there's much that can be done here, but let me know if you think of better names.


I'm not sure if there's a clean way to make this non-breaking or if it should instead become v4.

Alternatively, some extra code could maybe be shimmed that, rather than defaulting to a . delimiter, an empty (non-set) delimiter could parse the old way.

lmittmann commented 1 year ago

I was already preparing a PR to allow nested json configs with a simplified map traversal. This PR (https://github.com/jolheiser/ff/pull/1) adds that map traversal logic to all config parsers.

jolheiser commented 1 year ago

@lmittmann Ah sorry! I didn't mean to step on any toes, I should have responded to your issue as well. I implemented this per https://github.com/peterbourgon/ff/pull/108#issuecomment-1575189584

lmittmann commented 1 year ago

@jolheiser all good. Just wanted to pitch that idea to move the common code to internal. If you and @peterbourgon like you can include it. If not that's also fine. I created a PR for your PR

peterbourgon commented 1 year ago

I was already preparing a PR to allow nested json configs with a simplified map traversal. This PR (jolheiser#1) adds that map traversal logic to all config parsers.

I like this, more or less.

jolheiser commented 1 year ago

I've merged the other PR and added map[any]any to the switch-case for YAML.
afaik that just leaves the test changes you mentioned @peterbourgon

lmittmann commented 1 year ago

@peterbourgon will you merge this at some point? Or are there any changes required?

peterbourgon commented 1 year ago

I'm actively working on a larger refactor which includes these enhancements and several others. I expect to have a pre-release in the next few days.

jolheiser commented 1 year ago

Superseded by #112

Thanks @lmittmann and @peterbourgon!