Make file style to be colored with same structure while color_only #405 #436
Conversation
src/delta.rs
Outdated
| @@ -180,7 +188,7 @@ where | |||
| continue; | |||
| } | |||
|
|
|||
| if state == State::FileMeta && should_handle(&State::FileMeta, config) { | |||
| if state == State::FileMeta && should_handle(&State::FileMeta, config) && !config.color_only { | |||
There was a problem hiding this comment.
I think this can be refactored after commit style and hunk header style are also resolved.
| @@ -317,7 +325,7 @@ fn handle_generic_file_meta_header_line( | |||
| raw_line: &str, | |||
| config: &Config, | |||
| ) -> std::io::Result<()> { | |||
| if config.file_style.is_omitted { | |||
| if config.file_style.is_omitted && !config.color_only { | |||
| return Ok(()); | |||
| } | |||
There was a problem hiding this comment.
This is required, otherwise we need to refactor should_handle.
We can discuss and make another pr if we need.
There was a problem hiding this comment.
OK. Feel free to add explanatory comments to the code while it is fresh in your brain. If we have some test coverage of this then we'll feel even better about refactoring :)
There was a problem hiding this comment.
@dandavison
Add comment here def9554
| @@ -195,7 +195,6 @@ pub fn set_options( | |||
| opt.side_by_side = false; | |||
| opt.file_decoration_style = "none".to_string(); | |||
There was a problem hiding this comment.
This is required.
file_decoration_style will break structures. We should prevent any of them.
|
Great! Thanks for working on this @ulwlu.
You could open a second PR and use the PR target branch selector so that the second PR targets this branch. Unless I imagined it, I think GitHub is clever enough nowadays that the second one automatically switches to target master when the first merges. |
There was a problem hiding this comment.
This is great, there's just one thing I'd ask for -- do you think you could add a failing test that demonstrates the problem that you are fixing? If you don't mind rebasing, it's nice to have the failing test be the first commit and the fixes come after (then I probably won't use squash-and-merge because the history is useful.)
The utility ansi_test_utils::assert_line_has_style might be useful for the test.
|
@dandavison
Right, but I was not sure if this approach will be changed, and need to fix 2nd branch entirely or fix conflict (and I don't want so).
Sure, just a minute. |
|
Add test here. confirmed it's succeed now, and failed before the first commit. |
|
Perfect, thanks! Merged. |
This pr changes below.
I confirmed below apart from test.
git -c 'interactive.diffFilter="./target/release/delta" --color-only' add -pstill works.Remarks.
--color-onlyenabled #405commit-styleandhunk-header-style.commit-styleandhunk-header-style, but they depend on this pr.