bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#35564: [PATCH v4] Tweak dired warning about "wildcard" characters


From: Kévin Le Gouguec
Subject: bug#35564: [PATCH v4] Tweak dired warning about "wildcard" characters
Date: Sun, 28 Jul 2019 01:32:19 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux)

"Basil L. Contovounesios" <address@hidden> writes:

>> -(defun dired--no-subst-prompt (char-positions command)
>> +(defun dired--mark-positions (positions)
>> +  (let ((markers (make-string
>> +                  (1+ (apply #'max positions))
>
> Is POSITIONS guaranteed to be non-nil?  (The max function takes at least
> one argument.)

AFAICT dired--mark-positions is only called by dired--no-subst-prompt,
which is only used when there is at least one ambiguous character to
highlight.

So as things stand now, POSITIONS will always be non-nil.  Nothing
prevents someone from attempting to re-use the function with a
potentially-nil argument though.

I don't know what makes more sense here: adding an assertion?  Handling
the nil case explicitly for robustness?

>> Subject: [PATCH 6/6] Simplify highlighting assertions
>>
>> * test/lisp/dired-aux-tests.el (dired-test--check-highlighting):
>> New function.
>> (dired-test-highlight-metachar): Use it.
>
> Will this simplification hinder debugging of test failures?  I don't
> have an opinion on the proposed change, it's just something to consider.

Mmm.  Since the assertion that fails is now nested in a more generic
function, the report shown in the ERT-Results buffer might be somewhat
less informative; one has to bring up the backtrace to understand the
context.

I could try my hand at an ERT explainer for these assertions.  Or we
could just drop the 6th patch…  I do find the tests easier to read and
write with it though.


PS: Looking at this made me realize that patch #5 was borked (missed a
parenthesis in dired-test-highlight-metachar, so the tests just plain
wouldn't run).  Here is the patch series with patches #5 and #6 fixed.

The squashed patch[1] remains the same.

Attachment: 0001-Preserve-text-properties-in-y-or-n-p-prompts.patch
Description: Text Data

Attachment: 0002-Tweak-dired-warning-about-wildcard-characters.patch
Description: Text Data

Attachment: 0003-Dedup-dired-aux-isolated-char-searching-Bug-35564.patch
Description: Text Data

Attachment: 0004-fixup-Dedup-dired-aux-isolated-char-searching-Bug-35.patch
Description: Text Data

Attachment: 0005-Add-markers-below-non-isolated-chars-in-dired-prompt.patch
Description: Text Data

Attachment: 0006-Simplify-highlighting-assertions.patch
Description: Text Data

Thank you for your review.


[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28969#19

reply via email to

[Prev in Thread] Current Thread [Next in Thread]