Merge args into config #110
Conversation
| impl Config { | ||
| pub fn update(&mut self, source: &ConfigSource) { | ||
| if let Some(sign_commit) = source.sign_commit() { | ||
| self.sign_commit = Some(sign_commit); |
There was a problem hiding this comment.
why not just self.sign_commit = source.sign_commit().clone()
There was a problem hiding this comment.
Either way. Another option is to encapsulate this pattern with an extension trait.
There was a problem hiding this comment.
Thinking more about this...
The current approach references the struct twice with the proposed referencing it three times which increases the chance of error.
There was a problem hiding this comment.
Or is it possible to write a procedural macro for this? To avoid multiples changes required for adding new config option.
There was a problem hiding this comment.
Yes, a procedural macro could help here. Maybe I'm just not familiar enough with them but it seems like a big hammer to solve this small problem.
I do want to take these lessons back to the CLI-WG discussions. It feels like the current selection of crates is too all-in-one which doesn't let us adapt for the needs of crates like this one. I'm thinking a generalized set of macros to let people layer their config as needed would be helpful for the community.
There was a problem hiding this comment.
I had a clojure library, which I think deals this well. I'll think about the challenge to port it to static-typed language like rust.
There was a problem hiding this comment.
config and configure do it to a degree but I think they both use a HashMap when merging the sources which delays error reporting to the end. My goal with this was to report the error (invalid data type, extra field, etc) with being able to tell the user where the problem came from so they could fix it.
|
@epage do you want to improve on this branch, or I will merge it to let the process go |
If you are fine with it, I'd say let's get this in and continue to iterate. |
|
Yes. |
This is preparation for #93.