Skip to content
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 renaming types when generating optionals struct #2

Closed
wants to merge 3 commits into from

Conversation

carlosmn
Copy link
Contributor

@carlosmn carlosmn commented Oct 4, 2021

The goal here is to better support situations where you have something like

#[optfield(OptPorts)]
struct Ports {
    database: u16,
    auth: u16,
}

#[optfield(OptStats)]
struct Stats {
    port: u16,
    endpoint: Option<String>,
}

#[optfield(Opt)]
struct Options {
    ports: Ports,
    stats: Stats,
    daemonize: bool,
}

and don't want Option<Ports> but rather OptPorts in Opt.

This allows for e.g. a configuration file on disk to have the same structure and for everything to be optional recursively, instead of having to have these other structs be all-or-nothing and falling back to the defaults when we deserialise.

With these changes, you can specify

#[optfield(
    Opt,
    renames = (
        Ports = OptPorts,
        Stats = OptStats,
    )
)]
struct Options {
    ports: Ports,
    stats: Stats,
    daemonize: bool,
}

and the resulting struct can be merged from what we're able to read from disk without touching any of the fields that weren't specified in there.

I followed roughly the pattern that exists here with attrs where we specify everything at the head of the struct instead of e.g. specifying the attribute per field. But let me know if you think there's a better name or pattern.

One unfortunate restriction here is that if we use renames and merge_fn, all the functions have to be called the same as (IIUC) we don't (can't?) store what merge function name was used for the other structs. We could have the user specify them as part of the renames, but I figure that it's most likely that, if overriding at all, there would be a standard name used in a project so I wouldn't expect this to be an issue in practice.

This will let us specify that our embedded structs should become something else
instead of `Option<Struct>` to make it more useful with multiple levels of
structs we want to fill with `Option`s.
@carlosmn
Copy link
Contributor Author

Rebased to avoid the needless-borrow issue and now the tests pass, yay.

@roignpar
Copy link
Owner

roignpar commented Nov 2, 2021

Thank you and apologies for the late reply.

I agree it would be nice to cover this use case.
I don't like the idea of replacing a type with another type everywhere inside the struct. I'm thinking it would be nicer to do this per field using replace as the keyword:

#[optfield(
    Opt,
    replace = (
        ports = OptPorts, // "ports" being the field name
        stats = OptStats,
    )
)]
struct Options {
    ports: Ports,
    other_ports: Ports,
    stats: Stats,
    daemonize: bool,
}

// generated struct would look like
struct Opt {
    ports: Option<OptPorts>,
    other_ports: Option<Ports>,
    stats: Option<OptStats>,
    daemonize: Option<bool>
}

I feel like this is more flexible, even if it's inconsistent with how the rest of the macro arguments work.

This however doesn't solve the recursive merging functions issue.
I'm thinking the obvious solution would be:

#[optfield(
    Opt,
    replace = (
        // this would not recursively merge
        ports = OptPorts,
        // this would use the default merge_fn name for recursive merging
        other_ports = OptStats(merge_fn),
        // this would use "custom_merge_fn" for recursive merging
        stats = OptStats(merge_fn = custom_merge_fn)
    )
)]
struct Options {
    ports: Ports,
    other_ports: Ports,
    stats: Stats,
}

It feels a bit messy and I would like to give it some more thought.
In the meantime please feel free to make suggestions.

@carlosmn
Copy link
Contributor Author

carlosmn commented Nov 3, 2021

If we're going to list the replacements per-field, and somewhat different from the rest of options, then maybe we should go further and indicate the replacements next to them like e.g. serde does

struct Options {
     // this would not recursively merge
    #[replace = OptPorts]
    ports: Ports,
    // this would use the default merge_fn name for recursive merging
    #[replace = OptPorts(merge_fn)]
    other_ports: Ports,
    // this would use "custom_merge_fn" for recursive merging
    #[replace = OptState(merge_fn = custom_merge_fn)]
    stats: Stats,
}

which does change quite a bit from the original but it does look cleaner by moving the option right next to the use. The list can get quite big if we're storing everything at the top.

Alternatively you could have #[replace(OptPorts)] which I think looks more like other macros (but it makes the merge_fn stuff more different).

@roignpar
Copy link
Owner

@carlosmn In case you are still interested in this:
One feature of optfield is that it can be applied multiple times to the same struct.
Adding attributes to the fields themselves complicates things in such situations.
Either way, keeping the ability to apply optfield multiple times and adding type replacements would complicate the API quite a lot. I don't have the time nor interest to implement it at the moment.

It looks like this crate solves the type replacing issue: https://github.com/lesurp/OptionalStruct

Thank you for your effort. I'm sorry it didn't work out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants