diff_util: Move diff renderer format length check to constructor #6253
Conversation
In cli_util, I chose to put the `maybe_diff_renderer` function in the command environment instead of the helper because in `log.rs` there is no command helper already existing, and it would be non-trivial to build one first.
| if formats.is_empty() { | ||
| None | ||
| } else { |
There was a problem hiding this comment.
I would move this to WorkspaceCommandEnvironment::maybe_diff_renderer() or its callers.
There was a problem hiding this comment.
I like that it's part of the type's constructor. I think this should stay with the type, because it really is nonsensical to have a diff renderer that outputs nothing.
There was a problem hiding this comment.
It's not wrong to construct a diff function that does nothing. I think this could be a contract at cli_util layer, but as I said, I assumed it would be easier to test the condition by caller.
FWIW, we'll need to reorganize the current diff format configuration as "short" and "long" formats pair . (see DiffFormatArgs and #3327) DiffRenderer wouldn't have to know this distinction, but that might change how we construct the renderer object.
| .then(|| DiffRenderer::new(merged_repo, path_converter, conflict_marker_style, formats)) | ||
| }; | ||
| let formats = diff_formats_for_log(settings, &args.diff_format, args.patch)?; | ||
| let diff_renderer = workspace_env.maybe_diff_renderer(merged_repo, formats); |
There was a problem hiding this comment.
Perhaps, it's simpler to handle .is_empty() here instead of adding maybe_*() and wrapping the result?
(!formats.is_empty()).then(|| workspace_env.diff_renderer(merged_repo, formats));There was a problem hiding this comment.
diff_renderer only takes a single format, not a vector of formats.
You should try rewriting this if you want to see the pitfalls. I don't think a simpler solution really exists.
There was a problem hiding this comment.
I mean you can revert the change to accept multiple formats.
This is a nice simplification that I found in another commit that was fully unrelated to my pull request, so I'm breaking it out into a new one.
In cli_util, I chose to put the
maybe_diff_rendererfunction in the command environment instead of the helper because inlog.rsthere is no command helper already existing, and it would be non-trivial to build one first.Checklist
(I don't think any of these are applicable)
If applicable:
CHANGELOG.md