Refactor config-merging #147
Conversation
|
@tummychow, a work-in-progress. Mostly because I have ideas that I am not sure are compatible with yours, so here's a (functional) PR that at least starts a conversation. |
| @@ -730,15 +717,16 @@ lines | |||
|
|
|||
| become_new_author(&ctx); | |||
|
|
|||
| ctx.repo.config() | |||
| ctx.repo | |||
| .config() | |||
There was a problem hiding this comment.
As you can see, from a bonus commit that fixed bad formatting I previously introduced. You seem pragmatic, but I'm not sure how you feel about introducing such a bonus commit into this PR.
(As an aside, if keeping a good format is important to you (and I suspect it is, or the whole repo wouldn't've been formatted), is there interest in having a job that would check formatting when PRs come in? It could either prevent the merge or serve as a warning.)
There was a problem hiding this comment.
yeah, formatting in ci sgtm, and having this commit here is also fine with me
| ) | ||
| } | ||
|
|
||
| fn bool_value(repo: &Repository, setting_name: &str, default_value: bool) -> bool { |
There was a problem hiding this comment.
This seemed to me to be a reasonable start at deduping. Requires a different function per type of config data (and there's only one usize, so I didn't bother extracting). Happy to hear what you think could be improved.
| ), | ||
| force_author: config.force_author | ||
| || config.force | ||
| || bool_value(&repo, FORCE_AUTHOR_CONFIG_NAME, FORCE_AUTHOR_DEFAULT), |
There was a problem hiding this comment.
I took the liberty of inlining the above two calls to get the configuration data from git. The long constant names introduce wrapping, which maybe spoils the effect. If you prefer the methods back, happy to re-add.
| _ => ONE_FIXUP_PER_COMMIT_DEFAULT, | ||
| } | ||
| } | ||
|
|
||
| pub fn auto_stage_if_nothing_staged(repo: &git2::Repository) -> bool { |
There was a problem hiding this comment.
I wasn't sure if you'd want the values from these methods added to the Config object so we don't have to call them from throughout the code. Easy enough to do, if you like it, but introduces questions:
- do we want to expand
Configwith the extra attributes? - if not, how do you feel about a new struct that holds the unified settings? It wouldn't bloat
Config, and will eventually allow dropping of theforceattribute when it's only used to enable the otherforce_attributes - if you did like the idea of a second struct, naming becomes trickier; me, I'd avoid
Configfor either the new or existing struct (already taken by git's config) and probably look for name that indicate how the one struct comes from command-line arguments and the new one represents the unified settings. I'm just thinking out loud here.
There was a problem hiding this comment.
let's leave em as standalone functions for now. Config was originally intended to carry the cli arguments. i'm not tied to that interpretation of it, but no need to move everything in there atm
Ah, thanks. Mostly just concerned about matching your intent. Thanks for comments. Removed WIP from title. Merge away! |
Fixes #146.