Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draw ruler and box around headers - rebased #497

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

Svalorzen
Copy link

@Svalorzen Svalorzen commented Oct 18, 2024

This PR is the same as PR #398, I have just rebased it onto the current next so that there are no conflict. Just some re-tabbings were needed.

I'm doing this as the PR still seems to work well, and it would be a shame to lose the amount of work that @ericbn did.

Rulers are drawn around the first level of headers, and boxes around the
second level of headers, if any.
The `diff-so-fancy.rulerWidth` config still applies, but only to the
rulers. Boxes have just the size of the text inside them.
`git show` and `git log` have two levels of headers: (1) commit, and (2)
meta (e.g. `modified: diff-so-fancy`). `git diff` only has one level of
headers: meta.
This allows for a better hierarchical view of commits specifically, with
commit lines having a configurable wider ruler around them and meta
lines ("children" of commits) having a shorter box around them.
@scottchiefbaker
Copy link
Contributor

Couple of quick questions while I review this:

  1. Can you provide a screenshot of what the output before/after this change?
  2. Does this pass all the current unit tests?
  3. This is a pretty large change so this feature would need to be behind git config option so users can opt-in to it

@OJFord
Copy link
Member

OJFord commented Oct 19, 2024

@scottchiefbaker fwiw there's a comparison screenshot in the original PR: #398 (comment)

@Svalorzen
Copy link
Author

Svalorzen commented Oct 21, 2024

I have to admit I'm not really familiar with the codebase, nor I really know Perl. This is mostly a feature that I have wanted for a long time, and after seeing the older PR stay there for a while I just thought I'd give it a spin, and I didn't really do a lot of work to make it run again -- all credit goes to the original author.

AppVeyor seems to say that the build is failing, so I'm assuming some tests do fail; perhaps looking for full lines where partial lines get outputted with this patch. I think those tests did not exist when the patch was first proposed.

If there's anything I can do with my limited knowledge to help this be put in, I can try, but as I said I'm unfamiliar with both the language and the codebase so I'm limited in what I can offer.

@Svalorzen
Copy link
Author

In fact, I can already attest to a minor bug: if a file changed takes more than one line, the line wraps are incorrect.

Current:
image

With PR:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants