"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?

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