~ireas/public-inbox

1

[merge-rs] Feature requests

Joshua Nelson
Details
Message ID
<CAJ+j++X_eiep=A0tyQzEj7a9uX4Z43=-P9iXhDe5fBmRxkxvBg@mail.gmail.com>
DKIM signature
pass
Download raw message
Hello Robin Krahl,

Thanks for making merge-rs! I'm considering using it for the config
files used by rust compiler
(https://github.com/rust-lang/rust/pull/76628) and ran into a few
issues I was hoping you could help with.

Consider this struct:

```rust
#[derive(Deserialize, Merge)]
struct TomlConfig {
    build: Option<Build>,
    install: Option<Install>
}
```

Currently, `merge` will generate a derive that looks like this for it:

```rust
fn merge(&mut self, other: Self) {
    if self.build.is_none() { self.build = other.build; }
    if self.install.is_none() { self.install = other.install; }
}
```

However, this means that if you have configuration for both, the
config in `other` will be completely lost. I'd prefer the derive to
look more like this manual implementation I wrote, which preserves the
settings from `other` if they're present:

```rust
impl Merge for TomlConfig {
    fn merge(&mut self, other: Self) {
        fn do_merge<T: Merge>(x: &mut Option<T>, y: Option<T>) {
            if let Some(new) = y {
                if let Some(original) = x {
                    original.merge(new);
                } else {
                    *x = Some(new);
                }
            }
        };
        do_merge(&mut self.build, other.build);
        do_merge(&mut self.install, other.install);
    }
}
```

My second feature request is for `Merge` to work for hashmaps:

```rust
fn merge(original: &mut HashMap<String, TomlTarget>, other:
HashMap<String, TomlTarget>) {
    original.extend(other.into_iter());
}
```

Thank you again for working on `merge`! I'm not quite sure how to work
with sourcehut, but I'm happy to contribute code for this if you like.

Joshua Nelson
Details
Message ID
<20200920173250.GB1518@ireas.org>
In-Reply-To
<CAJ+j++X_eiep=A0tyQzEj7a9uX4Z43=-P9iXhDe5fBmRxkxvBg@mail.gmail.com> (view parent)
DKIM signature
pass
Download raw message
Hi Joshua,

thanks for your suggestions!

On 2020-09-20 12:12:29, Joshua Nelson wrote:
> Consider this struct:
> 
> ```rust
> #[derive(Deserialize, Merge)]
> struct TomlConfig {
>     build: Option<Build>,
>     install: Option<Install>
> }
> ```
> 
> Currently, `merge` will generate a derive that looks like this for it:
> 
> ```rust
> fn merge(&mut self, other: Self) {
>     if self.build.is_none() { self.build = other.build; }
>     if self.install.is_none() { self.install = other.install; }
> }
> ```
> 
> However, this means that if you have configuration for both, the
> config in `other` will be completely lost. I'd prefer the derive to
> look more like this manual implementation I wrote, which preserves the
> settings from `other` if they're present:
> 
> ```rust
> impl Merge for TomlConfig {
>     fn merge(&mut self, other: Self) {
>         fn do_merge<T: Merge>(x: &mut Option<T>, y: Option<T>) {
>             if let Some(new) = y {
>                 if let Some(original) = x {
>                     original.merge(new);
>                 } else {
>                     *x = Some(new);
>                 }
>             }
>         };
>         do_merge(&mut self.build, other.build);
>         do_merge(&mut self.install, other.install);
>     }
> }
> ```

This is a very interesting point that I was not aware of.  I agree that
the current behavior is counter-intuitive.

My approach so far was to only implement Merge directly for types where
a merge operation is unambigous.  Based on my use cases (command-line
arguments), I assumed that a merge for an option would always mean
overwriting a None value.  I realize that this was wrong.

For types where merge could have different implementations, I provide
merge strategies (i. e. functions with the signature Fn(&mut T, T)) that
can be selected using the strategy field of the merge attribute.

If I would change the Merge implementation for Option<T> to the code you
suggested, this would mean that something like this would no longer work
because String does not implement Merge:

    #[derive(Merge, StructOpt)]
    struct Args {
        #[structopt(short, long)]
        output_file: Option<String>,
    }

So I would suggest to solve this issue by removing the implementation of
Merge for Option<T> and instead adding two new merge strategies,
something like option::overwrite_none and option::merge_some.  This
would mean that you would have to write the TomlConfig struct as:

    #[derive(Deserialize, Merge)]
    struct TomlConfig {
        #[merge(strategy = merge::option::merge)]
        build: Option<Build>,
        #[merge(strategy = merge::option::merge)]
        install: Option<Install>
    }
 
 Would this be acceptable for you?  It might also be possible to set a
 default strategy for a struct to reduce the amount of boilerplate code,
 like this:

    #[derive(Deserialize, Merge)]
    #[merge(strategy = merge::option::merge)]
    struct TomlConfig {
        build: Option<Build>,
        install: Option<Install>
    }

> My second feature request is for `Merge` to work for hashmaps:
> 
> ```rust
> fn merge(original: &mut HashMap<String, TomlTarget>, other:
> HashMap<String, TomlTarget>) {
>     original.extend(other.into_iter());
> }
> ```

Good point.  Again, the problem is how to deal with values that are
present both in the original and in the other object.  My suggestion
would be to provide merge strategies that ignore, overwrite or merge
values with the same key.

> Thank you again for working on `merge`! I'm not quite sure how to work
> with sourcehut, but I'm happy to contribute code for this if you like.

Thanks!  The default Sourcehut workflow is to send patches using `git
send-email` to this mailing list, see this guide:
	https://git-send-email.io/
Alternativeley, if you want to keep it simple, you can just publish your
changes on GitHub or some other platform and send me a link to the
changes you want me to merge, either using your mail client or using
`git request-pull`.

Let me know if the new merge strategies would work for you and if you
want to implement them yourself or if I should add them.

Best,
Robin
Reply to thread Export thread (mbox)