lesurp / OptionalStruct

Macro copying a struct with Option fields. Useful for config initialization
Apache License 2.0
36 stars 12 forks source link

Allow nested objects to be Option<OptionalNested> #10

Closed dmoonfire closed 1 year ago

dmoonfire commented 5 years ago

To support working with deep nested files, it would be easier if every nested structure didn't have to be required. This adds an optional flag that allows for nested to be Option<OptionalNested> along with additional supporting code. There are tests for working with YAML (under a dev-dependency) and examples before and after.

This PR also allows other, non-optional-structure attributes like #[allow(dead_code)] and other attributes.

Also, a rustfmt before the development.

ttytm commented 1 year ago

Yes, this seems like a necessity, and shouldn't this be by default? It seems strange that when you create an optional struct, all fields are optional. And if you extend the optional struct to have its struct fields be nested as Options, the top level fields become required.

To speak in examples taken from the test:

#[derive(OptionalStruct)]
#[opt_nested_original(LogConfig)]
#[opt_nested_generated(OptionalLogConfig)]
struct Config {
    timeout: Option<u32>,
    log_config: LogConfig,
}

#[derive(OptionalStruct)]
struct LogConfig {
    log_file: String,
    log_level: usize,
}

will generate:

    let opt_config = OptionalConfig {
        timeout: None,
        log_config: OptionalLogConfig {
            log_file: Some("/tmp/bar.log".to_owned()),
            log_level: None,
        },
    };

This results in an error when e.g., reading a .toml config file if the file does not contain the log_config table, even though the table fields are optional an emtpy [log_config] table will still be required.

To fix this, something like the following would be necessary:

    let opt_config = OptionalConfig {
        timeout: None,
        log_config: Some(OptionalLogConfig {
            log_file: Some("/tmp/bar.log".to_owned()),
            log_level: None,
        }),
    };

@lesurp do you have a chance to take a look and maybe extend this functionality by merging this PR or implementing it as default? If you could top it off by updating the deps and releasing a new version it would be extra sweet. I would love to use it in an app.

Have a good New Year, cheers!

lesurp commented 1 year ago

Never realized some people actually used this crate! I'll take a look at all of this when I find some time.

lesurp commented 1 year ago

I think the branch refacto now implements this. I changed a bit the API, because I don't really like using derive macros for something generating structs (rather than simply implementing a trait), and it also makes passing extra arguments cleaner.

Looking at the new tests I added, one can do:

#[optional_struct]
struct Config {
    timeout: Option<u32>,

    #[optional_rename(OptionalLogConfig)]
    #[optional_skip_wrap]
    log_config: LogConfig,

The default behavior would generate a Option<LogConfig>, but the extra attribute makes sure we do not add the Option.

The default wrapping can be inverted:

#[optional_struct(OptionalConfig, false)]
struct Config {
    timeout: Option<u32>,

    #[optional_rename(OptionalLogConfig)]
    log_config: LogConfig,

    #[optional_wrap]
    baz: (),
}

I need to change the way I pass the attributes, and fix some warning about parenthesis (no idea why this is coming up), add tests etc.

ttytm commented 1 year ago

Looks great, thanks a lot!

What is your opinion about publishing an update on crates.io? Using a git repo as dependencies in a crate that should also be published on crates.io is unfortunately not possible.

lesurp commented 1 year ago

I just wanted to finish some more things before publishing it, here it is! https://crates.io/crates/optional_struct

Still have those annoying warnings when renaming nested structures but client code can easily suppress those for now.

ttytm commented 1 year ago

awesome :muscle:

tyvm!