feat(lib): merge-replace flag #39
Conversation
|
Hey! Thanks a lot for this contribution! I didn't have this need when I created this crate but I see why you'd need that! There are a few things that I think should be done differently. First, this introduces breaking changes. To avoid that, you could create a Second, I know there's no contribution section to inform you about that, but I'm trying to keep conventional commits (at least for the title of the PR that will be squashed) so could you rename it accordingly? Thanks again for your contribution, if you need some help, feel free to ask, I can jump in or even do the changes if needed. |
|
@jdrouet thanks for looking into it. I will amend PR and ping you when ready. |
evd0kim
changed the title
|
@evd0kim thanks for this update! Could you add a Also, quick question: was this generated with AI? |
|
HI @jdrouet. I've amended |
Raw version yes. I will add setter. |
|
Hi @jdrouet please have a look when you have a moment. I've amended Merger. |
src/lib.rs
Outdated
| pub fn merge_with_options( | ||
| value: Value, | ||
| other: Value, | ||
| replace_arrays: bool, | ||
| ) -> Result<Value, Error> { | ||
| let merger = Merger::new().with_replace_arrays(replace_arrays); | ||
| merger.merge(value, other) | ||
| } |
There was a problem hiding this comment.
Could we avoid introducing this function? It's not needed and is limiting considering it only takes replace_arrays parameter.
There was a problem hiding this comment.
No problem. Now it is merge_replace global function which allows merging with replacing arrays.
There was a problem hiding this comment.
I generally don't see the difference. Merge with options could be extended with other parameters. The only advantage is backward compatibility, probably.
There was a problem hiding this comment.
I don't think we need a merge_replace. Adding this extra shorthand might not be necessary considering the body of the function 🤔
There was a problem hiding this comment.
@jdrouet do you mean you don't like a public function and merging with replacement should happen via Merger instantiation and a method call? Because now I don't understand what API you'd like to see here.
There was a problem hiding this comment.
From my point of view, this kind of global function should have no option or a way to pass any function while not having a breaking change if a parameter is added.
If you want, you can have a merge_with_options but I don't think merge_replace has its place.
There was a problem hiding this comment.
Okay. I removed merge_replace and updated macro definition in tests.
jdrouet
added
the
AI Generated
Hello, for some reason we needed array replacement instead of extending them.
I suggest this PR for optional replacement of arrays provided in the inner toml.