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

[Basil L. Contovounesios]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> 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?

I think it's fine the way it is, though a docstring/comment never hurts.
I was just checking.

>>> 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.

That's good enough for me,

-- 
Basil