feat: Make RawConfig serializable

[Creative-Difficulty]

Related to #382.

I have implemented custom serializers based on the deserializers already in the code.

[Creative-Difficulty]

The test i added tests both serialization and deserialization, since the RawConfig can't be constructed programmatically. Should i rename it/the full_deserialize test or remove the full_deserialize test, since the new test incorporates its functionality?

[Creative-Difficulty]

@estk

[bconn98]

I would keep the tests, but maybe store the config as a static that all the tests can use instead of copy/pasting it

[Creative-Difficulty]

@bconn98 So the actual implementation is correct?

[Creative-Difficulty]

@bconn98 cfg is now a constant. Would you mind checking my implementation for correctness, as I didn't write any custom serializers, however the deserialize implementation uses a custom deserializer?

[Creative-Difficulty]

@bconn98

[bconn98]

@Creative-Difficulty So this is not good in it's current state. Due to how the library stores things, the default serialized configs can't be parsed by the library. Here is an example of printing out the output generated in your test.

refresh_rate:
  secs: 60
  nanos: 0
root:
  level: INFO
  appenders:
  - console
appenders:
  baz:
    kind: file
    filters: []
    config:
      encoder:
        pattern: '%m'
      path: /tmp/baz.log
  console:
    kind: console
    filters:
    - kind: threshold
      config:
        level: debug
    config: {}
loggers:
  foo::bar::baz:
    level: WARN
    appenders:
    - baz
    additive: false

Although we don't want to add PartialEq to everything in the library. For testing, you can add partial equal to RawConfig and other required components. Then try to parse you serialized config and compare the RawConfig from our static against the RawConfig deserialized from your serialized config.

[Creative-Difficulty]

@bconn98 I have a question: The humantime crate used for serializing/deserializing the refresh_rate option in RawConfig serializes 1 minute (as a Rust Duration) into the the string 1m. This means that de- and then reserializing the same config modifies it. I could hardcode m to be replaced with minute (or minutes when applicable) and so on with hours and days. Is this a cocern? Should I hardcode a translation as to not break the current examples?

[bconn98]

@Creative-Difficulty sounds like a translation is warranted here yup

[Creative-Difficulty]

@bconn98 In the latest commit serialization of filters and appenders is fixed. Currently 54 tests are passing with 1 failed and 1 ignored. The current output (of config::raw::test::full_serialize) is:

refresh_rate: 1m
root:
  level: INFO
  appenders:
  - console
appenders:
  baz:
    encoder:
      pattern: '%m'
    filters: '[]'
    kind: file
    path: /tmp/baz.log
  console:
    filters:
    - kind: threshold
      level: debug
    kind: console
loggers:
  foo::bar::baz:
    level: WARN
    appenders:
    - baz
    additive: false

However there are two issues:

1. Time formatting: Hardcoding a translation is a possibility, however in that case 60 seconds from the example(s) would be converted to 1m by the humantime crate, so the example config (const CFG) would still be modified if de- and then reserialized. In my opinion this defeats the whole point of trying not to modify the example configs when de- and reserializing them, so translations would be unnecessary from my point of view.

2. Indentation: The serde_yaml crate auto-indents its output with 2 spaces. Some issues (Yaml output indentation dtolnay/serde-yaml#337, Allow the indentation character to be customized dtolnay/serde-yaml#343) have already been opened, however I don't see anything being fixed/added soon since the repo is archived.

How do I proceed?

Thank you in advance!

[bconn98]

For #1, I think it's unfortunate but likely the best we can do with the existing crate backend. It's something we should include in documentation though. For #2, I'm not worried about the spacing as long as it's valid yaml.

[Creative-Difficulty]

@bconn98 So is the PR ready?

[Creative-Difficulty]

@bconn98 @estk @sfackler @demonov Is this PR ready for review/merge?

[bconn98]

All reviews are pending the fix for the deprecated chrono API

[izolyomi]

Thank you for the efforts. We'd also like to see this feature merged if possible.