bug#35018: 26.1; Use diff as en ert-explainer for string=

bug#35018: 26.1; Use diff as en ert-explainer for string=

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]


I've just committed webfeeder.el to ELPA.  In his review, Stefan Monnier
suggested we merged the following code snippet upstream:

--8<---------------cut here---------------start------------->8---
(defun webfeeder--string=-explainer (string-a string-b)
  "Return the diff output of STRING-A and STRING-B"
  (unless (string= string-a string-b)
    (let (file-a file-b)
      (unwind-protect
          (let (result)
            (setq file-a (make-temp-file "webfeeder")
                  file-b (make-temp-file "webfeeder"))
            (with-temp-file file-a
              (insert string-a))
            (with-temp-file file-b
              (insert string-b))
            (setq result
                  (with-temp-buffer
                    ;; The following generates a *Diff* buffer which is
                    ;; convenient for coloration.
                    (diff file-a file-b nil 'no-async)
                    (diff-no-select file-a file-b nil 'no-async (current-buffer))
                    (buffer-string)))
            result)
        (delete-file file-a)
        (delete-file file-b)))))

;; FIXME: Add this to ERT!
(put 'string= 'ert-explainer #'webfeeder--string=-explainer)
--8<---------------cut here---------------end--------------->8---

I've used this feature extensively in my ERT tests: it displays a "diff"
of string A and string B, which is a pretty nice default when the
strings are longer than a single line.

Food for thoughts :)

[-- Attachment #2.1: Type: text/plain, Size: 48 bytes --]


-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#35018: 26.1; Use diff as en ert-explainer for string=

[Noam Postavsky]

severity 35018 wishlist
quit

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> I've just committed webfeeder.el to ELPA.  In his review, Stefan Monnier
> suggested we merged the following code snippet upstream:
>
> (defun webfeeder--string=-explainer (string-a string-b)
>   "Return the diff output of STRING-A and STRING-B"
>   (unless (string= string-a string-b)

I guess a diff won't help so much for single line strings, so maybe the
condition should check for that? e.g.

    (or (string= string-a string-b)
        (not (string-match-p "\n" string-a))
        (not (string-match-p "\n" string-b))

>     (let (file-a file-b)
>       (unwind-protect
>           (let (result)
>             (setq file-a (make-temp-file "webfeeder")
>                   file-b (make-temp-file "webfeeder"))
>             (with-temp-file file-a
>               (insert string-a))
>             (with-temp-file file-b
>               (insert string-b))
>             (setq result
>                   (with-temp-buffer
>                     ;; The following generates a *Diff* buffer which is
>                     ;; convenient for coloration.
>                     (diff file-a file-b nil 'no-async)
>                     (diff-no-select file-a file-b nil 'no-async (current-buffer))

Isn't the diff-no-select redudant, since diff already calls it?

>                     (buffer-string)))
>             result)

You don't need the 'result' variable here, just make the
(with-temp-buffer...) the last expression in the let (which could then
be converted to progn).

>         (delete-file file-a)
>         (delete-file file-b)))))

You should check that each of file-a and file-b are non-nil before
trying to delete (in case either make-temp-file signals an error).

bug#35018: 26.1; Use diff as en ert-explainer for string=

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

Thanks for the review!

Noam Postavsky <npostavs@gmail.com> writes:

>> (defun webfeeder--string=-explainer (string-a string-b)
>>   "Return the diff output of STRING-A and STRING-B"
>>   (unless (string= string-a string-b)
>
> I guess a diff won't help so much for single line strings, so maybe the
> condition should check for that? e.g.
>
>     (or (string= string-a string-b)
>         (not (string-match-p "\n" string-a))
>         (not (string-match-p "\n" string-b))

Yes, this is very nice!

>>     (let (file-a file-b)
>>       (unwind-protect
>>           (let (result)
>>             (setq file-a (make-temp-file "webfeeder")
>>                   file-b (make-temp-file "webfeeder"))
>>             (with-temp-file file-a
>>               (insert string-a))
>>             (with-temp-file file-b
>>               (insert string-b))
>>             (setq result
>>                   (with-temp-buffer
>>                     ;; The following generates a *Diff* buffer which is
>>                     ;; convenient for coloration.
>>                     (diff file-a file-b nil 'no-async)
>>                     (diff-no-select file-a file-b nil 'no-async (current-buffer))
>
> Isn't the diff-no-select redudant, since diff already calls it?

The first diff is a typo.  It should be diff-no-select only.

Cheers!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#35018: 26.1; Use diff as en ert-explainer for string=

[Lars Ingebrigtsen]

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> I've just committed webfeeder.el to ELPA.  In his review, Stefan Monnier
> suggested we merged the following code snippet upstream:

I've adapted this to ert to see how it would look, and...  I'm not sure
it's all that helpful as is.  The following test:

(ert-deftest test-test-test ()
  (should (string= "foo\nbar" "zot\nbar")))

then gives this output:

    (ert-test-failed
     ((should
       (string= "foo\12bar" "zot\12bar"))
      :form
      (string= "foo\12bar" "zot\12bar")
      :value nil :explanation "diff -u /tmp/webfeeder1kS4io /tmp/webfeederwYoKdx\12--- /tmp/webfeeder1kS4io\0112021-06-23 15:37:54.407053381 +0200\12+++ /tmp/webfeederwYoKdx\0112021-06-23 15:37:54.407053381 +0200\12@@ -1,2 +1,2 @@\12-foo\12+zot\12 bar\12\\ No newline at end of file\12\12Diff finished.  Wed Jun 23 15:37:54 2021\12"))

Which isn't all that nice.

But I guess we could massage the output.  Removing the header and
trailer, we get:

    (ert-test-failed
     ((should
       (string= "foo\12bar" "zot\12bar"))
      :form
      (string= "foo\12bar" "zot\12bar")
      :value nil :explanation "@@ -1,2 +1,2 @@\12-foo\12+zot\12 bar\12\\ "))

But...  that's still not exactly readable.

So I don't know.  Anybody got an opinion here, or an easy fix that'll
make this readable in this context?

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index 6793b374ee..004bc81b4a 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -63,6 +63,7 @@
 (require 'ewoc)
 (require 'find-func)
 (require 'pp)
+(require 'diff)
 
 ;;; UI customization options.
 
@@ -535,6 +536,34 @@ ert--explain-equal
     (ert--explain-equal-rec a b)))
 (put 'equal 'ert-explainer 'ert--explain-equal)
 
+(defun ert--explain-string (string-a string-b)
+  "Return the diff output of STRING-A and STRING-B"
+  (unless (or (string= string-a string-b)
+              ;; Only do diffs if there are newlines.
+              (not (string-match-p "\n" string-a))
+              (not (string-match-p "\n" string-b)))
+    (let (file-a file-b)
+      (unwind-protect
+          (let (result)
+            (setq file-a (make-temp-file "ert")
+                  file-b (make-temp-file "ert"))
+            (with-temp-file file-a
+              (insert string-a))
+            (with-temp-file file-b
+              (insert string-b))
+            (setq result
+                  (with-temp-buffer
+                    ;; The following generates a *Diff* buffer which is
+                    ;; convenient for coloration.
+                    (diff-no-select file-a file-b nil 'no-async)
+                    (diff-no-select file-a file-b nil 'no-async
+                                    (current-buffer))
+                    (buffer-string)))
+            result)
+        (delete-file file-a)
+        (delete-file file-b)))))
+(put 'string= 'ert-explainer #'ert--explain-string)
+
 (defun ert--significant-plist-keys (plist)
   "Return the keys of PLIST that have non-null values, in order."
   (cl-assert (zerop (mod (length plist) 2)) t)


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#35018: 26.1; Use diff as en ert-explainer for string=

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

Hi Lars,

thanks for giving this a shot!

To clarify, the original intent was to display the output in a diff-mode
buffer, which would make it readable I believe.

Then no need to parse the output of the diff command :)

Cheers!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

bug#35018: 26.1; Use diff as en ert-explainer for string=

[Lars Ingebrigtsen]

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> To clarify, the original intent was to display the output in a diff-mode
> buffer, which would make it readable I believe.

Oh, I see...  So it shouldn't be displayed in batch runs at all?  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#35018: 26.1; Use diff as en ert-explainer for string=

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 288 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Oh, I see...  So it shouldn't be displayed in batch runs at all?  

The original implementation was not meant with batch mode in mind, but
we could make it more readable as you suggested.

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

bug#35018: 26.1; Use diff as en ert-explainer for string=

[Lars Ingebrigtsen]

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> The original implementation was not meant with batch mode in mind, but
> we could make it more readable as you suggested.

It's still not clear to me how this is supposed to be used.  Do you have
a more complete implementation, or a description of how this is to be
used in a workflow?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#35018: 26.1; Use diff as en ert-explainer for string=

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 129 bytes --]

Sorry, I don't have any other implementation and I haven't used it in
years, so I'm afraid I can't be of much help here.

Pierre

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

bug#35018: 26.1; Use diff as en ert-explainer for string=

[Lars Ingebrigtsen]

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> Sorry, I don't have any other implementation and I haven't used it in
> years, so I'm afraid I can't be of much help here.

OK -- it sounds like it's unlikely to be any further progress here in
this bug report, then, and I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no