Subject: Re: [bug#28832] [PATCH 1/3] gnu: Add emacs-json-reformat.

From: Oleg Pykhalov <[email-masked]>
Date: Tue, 12 Dec 2017 02:12:36 +0300

Hello Ludovic,

Apologies for late reply and thank you for review.

[email-masked] (Ludovic Courtès) writes:

[...]

>>> +       (modify-phases %standard-phases
>>> +         (add-before 'install 'check
>>> +           (lambda* (#:key inputs #:allow-other-keys)
>>> +             (zero? (system* "emacs" "--batch" "-L" "."
>>> +                             "-L" (string-append
>>> +                                   (assoc-ref inputs "emacs-undercover")
>>> +                                   "/share/emacs/site-lisp/guix.d/undercover-"
>>> +                                   ,(package-version emacs-undercover))
>>> +                             "-L" (string-append
>>> +                                   (assoc-ref inputs "emacs-dash")
>>> +                                   "/share/emacs/site-lisp/guix.d/dash-"
>>> +                                   ,(package-version emacs-dash))
>>> +                             "-L" (string-append
>>> +                                   (assoc-ref inputs "emacs-shut-up")
>>> +                                   "/share/emacs/site-lisp/guix.d/shut-up-"
>>> +                                   ,(package-version emacs-shut-up))
>>> +                             "-l" "test/test-helper.el"
>>> +                             "-l" "test/json-reformat-test.el"
>>> +                             "-f" "ert-run-tests-batch-and-exit"))
>>> +             ;; Fails
>>> +             ;; json-reformat-test:json-reformat-region-occur-error
>>> +             ;; json-reformat-test:string-to-string
>>> +             #t)))))
>>
>> Did you have a chance to investigate the test failures?  It’s not
>> confidence-inspiring ;-), so it would be good to at least have a link to
>> an upstream bug report.

Yes I looked at those tests.  Here is a little review of them.

json-reformat-test:string-to-string basically just calls
a reimplemented in Emacs 25 json-encode-string function.

json-reformat-test:json-reformat-region-occur-error emulates error and
produce a message.  This message is differ from Emacs 24 in symbol '`'.

So, I think those test fails are harmless.  I attach the new patch with
fixing those failing tests.

From d589de21acb02d0fba7aee3b48e5f42d7bd8957e Mon Sep 17 00:00:00 2001
From: Oleg Pykhalov <[email-masked]>
Date: Tue, 12 Dec 2017 01:41:08 +0300
Subject: [PATCH] gnu: Add emacs-json-reformat.

* gnu/packages/patches/emacs-json-reformat-fix-tests.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add this.
* gnu/packages/emacs.scm (emacs-json-reformat): New variable.
---
 gnu/local.mk                                       |  1 +
 gnu/packages/emacs.scm                             | 47 ++++++++++++++++++++++
 .../patches/emacs-json-reformat-fix-tests.patch    | 28 +++++++++++++
 3 files changed, 76 insertions(+)
 create mode 100644 gnu/packages/patches/emacs-json-reformat-fix-tests.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 46829756b..ec5ba06d3 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -613,6 +613,7 @@ dist_patch_DATA =						\
   %D%/packages/patches/einstein-build.patch			\
   %D%/packages/patches/emacs-exec-path.patch			\
   %D%/packages/patches/emacs-fix-scheme-indent-function.patch	\
+  %D%/packages/patches/emacs-json-reformat-fix-tests.patch	\
   %D%/packages/patches/emacs-highlight-stages-add-gexp.patch	\
   %D%/packages/patches/emacs-scheme-complete-scheme-r5rs-info.patch	\
   %D%/packages/patches/emacs-source-date-epoch.patch		\
diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
index fd0305629..1aa139c92 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -5914,6 +5914,53 @@ pair of minor modes which suppress all mouse events by intercepting them and
 running a customisable handler command (@code{ignore} by default). ")
     (license license:gpl3+)))
 
+(define-public emacs-json-reformat
+  (package
+    (name "emacs-json-reformat")
+    (version "0.0.6")
+    (source
+     (origin
+       (method url-fetch)
+       (uri (string-append "https://github.com/gongo/json-reformat/archive/"
+                           version ".tar.gz"))
+       (file-name (string-append name "-" version ".tar.gz"))
+       (sha256
+        (base32
+         "11fbq4scrgr7m0iwnzcrn2g7xvqwm2gf82sa7zy1l0nil7265p28"))
+       (patches (search-patches "emacs-json-reformat-fix-tests.patch"))))
+    (build-system emacs-build-system)
+    (propagated-inputs `(("emacs-undercover" ,emacs-undercover)))
+    (inputs
+     `(("emacs-dash" ,emacs-dash)         ; for tests
+       ("emacs-shut-up" ,emacs-shut-up))) ; for tests
+    (arguments
+     `(#:phases
+       (modify-phases %standard-phases
+         (add-before 'install 'check
+           (lambda* (#:key inputs #:allow-other-keys)
+             (zero? (system* "emacs" "--batch" "-L" "."
+                             "-L" (string-append
+                                   (assoc-ref inputs "emacs-undercover")
+                                   "/share/emacs/site-lisp/guix.d/undercover-"
+                                   ,(package-version emacs-undercover))
+                             "-L" (string-append
+                                   (assoc-ref inputs "emacs-dash")
+                                   "/share/emacs/site-lisp/guix.d/dash-"
+                                   ,(package-version emacs-dash))
+                             "-L" (string-append
+                                   (assoc-ref inputs "emacs-shut-up")
+                                   "/share/emacs/site-lisp/guix.d/shut-up-"
+                                   ,(package-version emacs-shut-up))
+                             "-l" "test/test-helper.el"
+                             "-l" "test/json-reformat-test.el"
+                             "-f" "ert-run-tests-batch-and-exit"))
+             #t)))))
+    (home-page "https://github.com/gongo/json-reformat")
+    (synopsis "Reformatting tool for JSON")
+    (description "@code{json-reformat} provides a reformatting tool for
+@url{http://json.org/, JSON}.")
+    (license license:gpl3+)))
+
 (define-public emacs-json-snatcher
   (package
     (name "emacs-json-snatcher")
diff --git a/gnu/packages/patches/emacs-json-reformat-fix-tests.patch b/gnu/packages/patches/emacs-json-reformat-fix-tests.patch
new file mode 100644
index 000000000..23a239582
--- /dev/null
+++ b/gnu/packages/patches/emacs-json-reformat-fix-tests.patch
@@ -0,0 +1,28 @@
+Copyright © 2017 Oleg Pykhalov <[email-masked]>
+
+This patch fixes tests for Emacs 25.
+
+diff --git a/test/json-reformat-test.el b/test/json-reformat-test.el
+index 7de3be1..b4a4dde 100644
+--- a/test/json-reformat-test.el
++++ b/test/json-reformat-test.el
+@@ -58,7 +58,7 @@
+ (ert-deftest json-reformat-test:string-to-string ()
+   (should (string= "\"foobar\"" (json-reformat:string-to-string "foobar")))
+   (should (string= "\"fo\\\"o\\nbar\"" (json-reformat:string-to-string "fo\"o\nbar")))
+-  (should (string= "\"\\u2661\"" (json-reformat:string-to-string "\u2661")))
++  (should (string= "\"♡\"" (json-reformat:string-to-string "\u2661")))
+ 
+   (should (string= "\"^(amq\\\\.gen.*|amq\\\\.default)$\"" (json-reformat:string-to-string "^(amq\\.gen.*|amq\\.default)$")))
+   )
+@@ -148,6 +148,6 @@ bar\"" (json-reformat:string-to-string "fo\"o\nbar")))
+ [{ foo : \"bar\" }, { \"foo\" : \"baz\" }]") ;; At 3 (line)
+         (json-reformat-region (point-min) (point-max)))
+       (should (string=
+-               "JSON parse error [Reason] Bad string format: \"doesn't start with '\\\"'!\" [Position] In buffer, line 3 (char 6)"
++               "JSON parse error [Reason] Bad string format: \"doesn't start with \`\\\"'!\" [Position] In buffer, line 3 (char 6)"
+                message-string))
+       )))
+-- 
+2.15.1
+
-- 
2.15.1

The upstream is quite: https://github.com/gongo/json-reformat/issues/33

>> Besides, I wonder: shouldn’t ‘emacs-build-system’ define the
>> ‘EMACSLOADPATH’ env. var. so we don’t have to carry all these -L flags?
>> Is there any downside?

As I see from the documentation¹ EMACSLOADPATH is a list of directories
with *.el files in it.  If we will use it, then it will be almost the
same carring bunch of directories in package recipes, will it?

¹ https://www.gnu.org/software/emacs/manual/html_node/emacs/General-Variables.html

Oleg.