[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] Font lock long Git commit summary lines
From: |
Eli Zaretskii |
Subject: |
Re: [PATCH v2] Font lock long Git commit summary lines |
Date: |
Mon, 05 Sep 2022 15:07:49 +0300 |
> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Sun, 04 Sep 2022 16:22:29 -0700
>
> Here is v2 of my patch. Thank you for the comments.
Thanks. May I comment a bit more?
> +(defcustom vc-git-log-edit-summary-target nil
> + "Target length for Git commit summary lines.
Given the description below, "Target length" is not the best wording
here. Perhaps "Preferred maximum length" is better? And
consequently, I think a better name for the variable is
vc-git-log-edit-summary-warning-len
> +By setting this to an integer around 50, you can improve the
> +compatibility of your commit messages with Git commands that
> +print the summary line in width-constrained contexts. However,
> +many commit summaries will need to exceed this length.
I'd lose the last sentence here: it confuses the whole issue. When
you say "Preferred" or "Target", and describe what will happen beyond
that, you already in effect tell the reader that the limitation is not
a hard one, but just a guideline.
> +(defface vc-git-log-edit-summary-warning
> + '((t :inherit warning))
> + "Face for Git commit summary lines beyond the target length.")
This should mention vc-git-log-edit-summary-target. Imagine a user
who looks at the face's doc string and wonders which "target length"
does the above allude to.
> +(defcustom vc-git-log-edit-summary-max 68
I'd call this vc-git-log-edit-summary-max-len
> + "Maximum length for Git commit summary lines.
> +If non-nil, characters in summary lines beyond this length are
^^^^^^^^^^
"If a number, ..." Saying "if non-nil, ... beyond this length" is
problematic, because not all non-nil values are numbers.
> +(defface vc-git-log-edit-summary-error
> + '((t :inherit error))
> + "Face for Git commit summary lines beyond the maximum length.")
Reference the variable in the doc string.
> +(defun vc-git--log-edit-summary-check (limit)
> + (and (re-search-forward "^Summary: " limit t)
> + (when-let ((regex
> + (cond ((and vc-git-log-edit-summary-max
> + vc-git-log-edit-summary-target)
Should this test using numberp?
> + (vc-git-log-edit-summary-max
> + (format ".\\{,%d\\}\\(?2:.*\\)"
> + vc-git-log-edit-summary-max))
> + (vc-git-log-edit-summary-target
> + (format ".\\{,%d\\}\\(.*\\)"
> + vc-git-log-edit-summary-target)))))
Likewise here.