fix/ert-multiline-explanation

fix/ert-multiline-explanation

[Phillip Lord]

I would appreciate feedback on whether the change on
fix/ert-multiline-explanation would be a good one.

At the moment, ert allows you to attach explanation functions to explain
why tests have failed. Unfortunately, these explanations are printed out
using "pp" which escapes new lines. So multiline explanations are,
largely, unreadable.

So this patch ignores the value of pp-escape-newlines and resets it to
nil for the duration.

Re: fix/ert-multiline-explanation

[David Kastrup]

phillip.lord@russet.org.uk (Phillip Lord) writes:

> I would appreciate feedback on whether the change on
> fix/ert-multiline-explanation would be a good one.
>
> At the moment, ert allows you to attach explanation functions to explain
> why tests have failed. Unfortunately, these explanations are printed out
> using "pp" which escapes new lines. So multiline explanations are,
> largely, unreadable.
>
> So this patch ignores the value of pp-escape-newlines and resets it to
> nil for the duration.

This patch?

-- 
David Kastrup

Re: fix/ert-multiline-explanation

[Phillip Lord]

David Kastrup <dak@gnu.org> writes:

> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>> I would appreciate feedback on whether the change on
>> fix/ert-multiline-explanation would be a good one.
>>
>> At the moment, ert allows you to attach explanation functions to explain
>> why tests have failed. Unfortunately, these explanations are printed out
>> using "pp" which escapes new lines. So multiline explanations are,
>> largely, unreadable.
>>
>> So this patch ignores the value of pp-escape-newlines and resets it to
>> nil for the duration.
>
> This patch?

On the branch fix/ert-multiline-explanation. 

a23aa4c9c99147bd6f36941fbae3a2d94c481bed

http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=fix/ert-multiline-explanation&id=a23aa4c9c99147bd6f36941fbae3a2d94c481bed

Never really understood patches. Branches or request pull/pull
request/merge request seem much easier.

Phil

Re: fix/ert-multiline-explanation

[Phillip Lord]

Phillip Lord <phillip.lord@russet.org.uk> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> phillip.lord@russet.org.uk (Phillip Lord) writes:
>>
>>> I would appreciate feedback on whether the change on
>>> fix/ert-multiline-explanation would be a good one.
>>>
>>> At the moment, ert allows you to attach explanation functions to explain
>>> why tests have failed. Unfortunately, these explanations are printed out
>>> using "pp" which escapes new lines. So multiline explanations are,
>>> largely, unreadable.
>>>
>>> So this patch ignores the value of pp-escape-newlines and resets it to
>>> nil for the duration.
>>
>> This patch?
>
> On the branch fix/ert-multiline-explanation. 
>
> a23aa4c9c99147bd6f36941fbae3a2d94c481bed
>
> http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=fix/ert-multiline-explanation&id=a23aa4c9c99147bd6f36941fbae3a2d94c481bed
>
> Never really understood patches. Branches or request pull/pull
> request/merge request seem much easier.

Any comments?

Phil

Re: fix/ert-multiline-explanation

[David Engster]

Phillip Lord writes:
>>>> I would appreciate feedback on whether the change on
>>>> fix/ert-multiline-explanation would be a good one.

> Any comments?

I would definitely be in favor of that change. The backtraces in its
current form are pretty much unreadable.

-David

Re: fix/ert-multiline-explanation

[Phillip Lord]

David Engster <deng@randomsample.de> writes:

> Phillip Lord writes:
>>>>> I would appreciate feedback on whether the change on
>>>>> fix/ert-multiline-explanation would be a good one.
>
>> Any comments?
>
> I would definitely be in favor of that change. The backtraces in its
> current form are pretty much unreadable.


It doesn't change the backtraces AFAICT.

Phil

Re: fix/ert-multiline-explanation

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Date: Wed, 21 Oct 2015 21:43:14 +0100
> 
> 
> I would appreciate feedback on whether the change on
> fix/ert-multiline-explanation would be a good one.
> 
> At the moment, ert allows you to attach explanation functions to explain
> why tests have failed. Unfortunately, these explanations are printed out
> using "pp" which escapes new lines. So multiline explanations are,
> largely, unreadable.
> 
> So this patch ignores the value of pp-escape-newlines and resets it to
> nil for the duration.

Could you please show an example, with and without the changes?

TIA

Re: fix/ert-multiline-explanation

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Date: Wed, 21 Oct 2015 21:43:14 +0100
>> 
>> 
>> I would appreciate feedback on whether the change on
>> fix/ert-multiline-explanation would be a good one.
>> 
>> At the moment, ert allows you to attach explanation functions to explain
>> why tests have failed. Unfortunately, these explanations are printed out
>> using "pp" which escapes new lines. So multiline explanations are,
>> largely, unreadable.
>> 
>> So this patch ignores the value of pp-escape-newlines and resets it to
>> nil for the duration.
>
> Could you please show an example, with and without the changes?


Sure. The following code achieves the same thing with advice.

(defun silly-predicate (x))

(defun silly-explainer (&rest args)
  (message "Silly predicate is silly.
I mean, it's in the name, so why are you using it?
It's never going to return a sensible answer.
It is after silly.
            ^^^^^"))

(put 'silly-predicate
     'ert-explainer
     'silly-explainer)

(defun sisyphus--ert-pp-with-indentation-and-newline (orig object)
  (let ((pp-escape-newlines nil))
    (funcall orig object)))

(ert-deftest with ()
  (should
   (silly-predicate
    (advice-add
     'ert--pp-with-indentation-and-newline
     :around
     #'sisyphus--ert-pp-with-indentation-and-newline))))

(ert-deftest without ()
  (should
   (silly-predicate
    (advice-remove
     'ert--pp-with-indentation-and-newline
     #'sisyphus--ert-pp-with-indentation-and-newline))))


And the output.



F with
    (ert-test-failed
     ((should
       (silly-predicate
        (advice-add 'ert--pp-with-indentation-and-newline :around #'sisyphus--ert-pp-with-indentation-and-newline)))
      :form
      (silly-predicate nil)
      :value nil :explanation "Silly predicate is silly.
I mean, it's in the name, so why are you using it?
It's never going to return a sensible answer.
It is after silly.
            ^^^^^"))

F without
    (ert-test-failed
     ((should
       (silly-predicate
        (advice-remove 'ert--pp-with-indentation-and-newline #'sisyphus--ert-pp-with-indentation-and-newline)))
      :form
      (silly-predicate nil)
      :value nil :explanation "Silly predicate is silly.\nI mean, it's in the name, so why are you using it?\nIt's never going to return a sensible answer.\nIt is after silly.\n            ^^^^^"))



The point is with the advice I can use multiline explainers, without it
I can but it's pointless.

The bug fix is not essential -- as I have shown I can advice around it,
for this use case, I am struggling to see a use case for which "\n" is
better than a newline.

Phil

Re: fix/ert-multiline-explanation

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: <emacs-devel@gnu.org>
> Date: Mon, 26 Oct 2015 08:55:12 +0000
> 
> F with
>     (ert-test-failed
>      ((should
>        (silly-predicate
>         (advice-add 'ert--pp-with-indentation-and-newline :around #'sisyphus--ert-pp-with-indentation-and-newline)))
>       :form
>       (silly-predicate nil)
>       :value nil :explanation "Silly predicate is silly.
> I mean, it's in the name, so why are you using it?
> It's never going to return a sensible answer.
> It is after silly.
>             ^^^^^"))
> 
> F without
>     (ert-test-failed
>      ((should
>        (silly-predicate
>         (advice-remove 'ert--pp-with-indentation-and-newline #'sisyphus--ert-pp-with-indentation-and-newline)))
>       :form
>       (silly-predicate nil)
>       :value nil :explanation "Silly predicate is silly.\nI mean, it's in the name, so why are you using it?\nIt's never going to return a sensible answer.\nIt is after silly.\n            ^^^^^"))

Looks like your change is a clear improvement, so please install.

Thanks.

Re: fix/ert-multiline-explanation

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> F with
>>     (ert-test-failed
>>      ((should
>>        (silly-predicate
>>         (advice-add 'ert--pp-with-indentation-and-newline :around #'sisyphus--ert-pp-with-indentation-and-newline)))
>>       :form
>>       (silly-predicate nil)
>>       :value nil :explanation "Silly predicate is silly.
>> I mean, it's in the name, so why are you using it?
>> It's never going to return a sensible answer.
>> It is after silly.
>>             ^^^^^"))
>
> Looks like your change is a clear improvement, so please install.
>


All done, thanks for the feedback!

Phil