GNU bug report logs - #12634
24.2; [FEATURE REQUEST] JSON pretty printer

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 12634 in the body.
You can then email your comments to 12634 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Leo <sdl.web <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.2; [FEATURE REQUEST] JSON pretty printer
Date: Sat, 13 Oct 2012 16:39:25 +0800

It would be lovely if the included json.el could do pretty printing.

Message #8 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Leo <sdl.web <at> gmail.com>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: 24.2; [FEATURE REQUEST] JSON pretty printer
Date: Sat, 13 Oct 2012 17:56:10 -0400

> It would be lovely if the included json.el could do pretty printing.

Patch welcome,


        Stefan

Message #11 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Aurélien Aptel <aurelien.aptel <at> gmail.com>
To: 12634 <at> debbugs.gnu.org
Subject: existing implementation
Date: Thu, 18 Oct 2012 23:18:43 +0200

Someone already made a pretty-print patch [1] and other utility functions [2].

1: https://github.com/thorstadt/json.el
2: https://github.com/thorstadt/json-pretty-print.el

Message #14 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Aurélien Aptel <aurelien.aptel <at> gmail.com>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: existing implementation
Date: Thu, 18 Oct 2012 21:37:39 -0400

> Someone already made a pretty-print patch [1] and other utility
> functions [2].
> 1: https://github.com/thorstadt/json.el
> 2: https://github.com/thorstadt/json-pretty-print.el

Would you be so kind and try to see if we can get the copyright cleared?


        Stefan

Message #17 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Aurélien Aptel <aurelien.aptel <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: existing implementation
Date: Fri, 19 Oct 2012 18:20:33 +0200

On Fri, Oct 19, 2012 at 3:37 AM, Stefan Monnier
<monnier <at> iro.umontreal.ca> wrote:
> Would you be so kind and try to see if we can get the copyright cleared?

I've asked him via email (mentioning the url to this bug). I've also
sent the instructions to get the assignment form.

Message #20 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Aurélien Aptel <aurelien.aptel <at> gmail.com>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: existing implementation
Date: Fri, 19 Oct 2012 14:04:49 -0400

>> Would you be so kind and try to see if we can get the copyright cleared?
> I've asked him via email (mentioning the url to this bug). I've also
> sent the instructions to get the assignment form.

Great, thank you,


        Stefan

Message #23 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Aurélien Aptel <aurelien.aptel <at> gmail.com>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: existing implementation
Date: Fri, 19 Oct 2012 17:28:16 -0400

>>> Would you be so kind and try to see if we can get the copyright cleared?
>> I've asked him via email (mentioning the url to this bug). I've also
>> sent the instructions to get the assignment form.
> Great, thank you,

BTW, please make sure that he specifies "Emacs" (and not json.el for
example) as the package for which he wants to sign the paperwork.


        Stefan

Message #26 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Ryan Crum <ryan.j.crum <at> gmail.com>
To: 12634 <at> debbugs.gnu.org
Subject: Patch for pretty-printing in json.el
Date: Thu, 25 Oct 2012 10:55:39 -0400

[Message part 1 (text/plain, inline)]

Hi,

I've consolidated my changes into json.el. I have not yet received the form
for copyright assignment in the mail, but I have requested it. Please
advise me if you'd like me to make any changes here, and I'll be happy to
accomodate. Patch attached.

Thanks,

-Ryan

[Message part 2 (text/html, inline)]

[pretty-printing-patch (application/octet-stream, attachment)]

Message #29 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Ryan Crum <ryan.j.crum <at> gmail.com>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: Patch for pretty-printing in json.el
Date: Thu, 25 Oct 2012 14:08:59 -0400

> I've consolidated my changes into json.el.  I have not yet received
> the form for copyright assignment in the mail, but I have
> requested it.

Great, thanks.

> Please advise me if you'd like me to make any changes here, and I'll
> be happy to accomodate.  Patch attached.

It looks OK overall, but I do have some comments:
- it would be better not to re-compute json-encoding-current-separator
  every time we call json-encode, since that function is called all
  the time.
  IOW, build it once in an external caller.  Or better yet: get rid of
  json-encoding-current-separator and add a "\n" at the beginning of
  json-encoding-current-indentation instead.
- you can use the "json--" prefix to indicate it is an internal
  variable/function.
- You could also prefer to place the closing ] at the end of the
  previous line, à la Lisp ;-)


        Stefan

Message #32 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Ryan Crum <ryan.j.crum <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: Patch for pretty-printing in json.el
Date: Sat, 27 Oct 2012 15:31:46 -0400

[Message part 1 (text/plain, inline)]

Cool, new patch attached. I've consolidated current-separator into
current-indentation and created a little private helper function
`json--current-whitespace' for the newline/indentation.

I've also created a var called `json-encoding-lisp-style-closings' per your
request. :-)

Just let me know if there's anything else.

Thanks,

-Ryan

On Thu, Oct 25, 2012 at 2:08 PM, Stefan Monnier <monnier <at> iro.umontreal.ca>wrote:

> It looks OK overall, but I do have some comments:
> - it would be better not to re-compute json-encoding-current-separator
>   every time we call json-encode, since that function is called all
>   the time.
>   IOW, build it once in an external caller.  Or better yet: get rid of
>   json-encoding-current-separator and add a "\n" at the beginning of
>   json-encoding-current-indentation instead.
> - you can use the "json--" prefix to indicate it is an internal
>   variable/function.
> - You could also prefer to place the closing ] at the end of the
>   previous line, à la Lisp ;-)
>
>
>         Stefan
>

[Message part 2 (text/html, inline)]

[json-pretty-printing (application/octet-stream, attachment)]

Message #35 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Ryan Crum <ryan.j.crum <at> gmail.com>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: Patch for pretty-printing in json.el
Date: Tue, 30 Oct 2012 16:03:11 -0400

> Cool, new patch attached. I've consolidated current-separator into
> current-indentation and created a little private helper function
> `json--current-whitespace' for the newline/indentation.

Thanks.  More questions/remarks:

- Your patch does not apply to the trunk version of json.el where
  alist/plist keys are encoded with a new json-encode-key.

- I don't understand this helper function.  Why not store the leading "\n"
  directly in json-encoding-current-indentation so that
  we can use json-encoding-current-indentation directly instead of
  calling json--current-whitespace?

- BTW your patch calls json-encoding-current-indentation as a function in
  json-encode-plist.

- OTOH, I wouldn't mind a helper function/macro to consolidate all the

          (let ((json-encoding-current-indentation
                 (if json-encoding-pretty-print
                     (concat json-encoding-current-indentation
                             json-encoding-default-indentation)
                   "")))

  in a single spot.

- Why use ", " rather than "," for json-encoding-default-separator?

- json-encoding-default-separator is a bad name since it holds the
  *current* separator, rather than the default.

- Why (format "%s" (json--current-whitespace)) rather than
  (json--current-whitespace)?

> I've also created a var called `json-encoding-lisp-style-closings' per your
> request. :-)

Thanks.

> Just let me know if there's anything else.

I think that's enough nitpicking for now.


        Stefan

Message #38 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Ryan Crum <ryan.j.crum <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: Patch for pretty-printing in json.el
Date: Tue, 30 Oct 2012 17:04:52 -0400

[Message part 1 (text/plain, inline)]

OK, let's try this again.

New patch attached.

On Tue, Oct 30, 2012 at 4:03 PM, Stefan Monnier <monnier <at> iro.umontreal.ca>wrote:

> - Your patch does not apply to the trunk version of json.el where
>   alist/plist keys are encoded with a new json-encode-key.
>

Oops. Fixed.


>
> - I don't understand this helper function.  Why not store the leading "\n"
>   directly in json-encoding-current-indentation so that
>   we can use json-encoding-current-indentation directly instead of
>   calling json--current-whitespace?
>

Yeah, I see what you mean. Fixed.


>
> - BTW your patch calls json-encoding-current-indentation as a function in
>   json-encode-plist.
>

Foggy-headed. Fixed.


>
> - OTOH, I wouldn't mind a helper function/macro to consolidate all the
>
>           (let ((json-encoding-current-indentation
>                  (if json-encoding-pretty-print
>                      (concat json-encoding-current-indentation
>                              json-encoding-default-indentation)
>                    "")))
>
>   in a single spot.
>

I created a little macro for this (json--with-indentation).


>
> - Why use ", " rather than "," for json-encoding-default-separator?
>

Vestigial code from a much older version of this patch. Fixed.


>
> - json-encoding-default-separator is a bad name since it holds the
>   *current* separator, rather than the default.
>

Renamed to json-encoding-separator. This was also vestigial (at one point
it only used line-breaks if lists were beyond a certain length; overly
complex).


> - Why (format "%s" (json--current-whitespace)) rather than
>   (json--current-whitespace)?
>

Yeah, good question. Fixed.

[Message part 2 (text/html, inline)]

[json-pretty-printing.diff (application/octet-stream, attachment)]

Message #41 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Ryan Crum <ryan.j.crum <at> gmail.com>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: Patch for pretty-printing in json.el
Date: Wed, 14 Nov 2012 20:56:30 -0500

> OK, let's try this again.
> New patch attached.

Getting there.  But still waiting for the copyright paperwork.

Since we still have time, here are some further nitpicks.

> @@ -99,6 +100,25 @@
>  tell the difference between `false' and `null'.  Consider let-binding
>  this around your call to `json-read' instead of `setq'ing it.")
 
> +(defvar json-encoding-separator ","
> +  "Value to use as an element seperator when encoding.")
> +
> +(defvar json-encoding-default-indentation "  "
> +  "The default indentation level for encoding. Used only when
> +`json-encoding-pretty-print' is non-nil.")
> +
> +(defvar json-encoding-current-indentation "\n"
> +  "Internally used to keep track of the current indentation level of
> +encoding. Used only when `json-encoding-pretty-print' is non-nil.")

Use a "json--" prefix, since it's a convention we use to express that
something is internal.

> +(defvar json-encoding-pretty-print nil
> +  "Setting this to non-nil will result in the output of `json-encode'
> +to be pretty-printed.")
> +
> +(defvar json-encoding-lisp-style-closings nil
> +  "Setting this to `t' will cause ] and } closings to happen lisp-style,
> +without indentation.")

The first line of a docstring should "stand on it own", i.e. it
shouldn't end in the middle of a sentence.
Try C-u M-x checkdoc-current-buffer.

Rather than "Setting this to <foo> will cause <bar>", we usually just
say "If <foo>, <bar>".  E.g.
  "If non-nil, the output of `json-encode' will be pretty-printed."

Also, contrary to all other symbols, t (like nil) is written without
`...' quoting.
  
> +(defmacro json--with-indentation (body)
> +  `(let ((json-encoding-current-indentation
> +          (if json-encoding-pretty-print
> +              (concat json-encoding-current-indentation
> +                      json-encoding-default-indentation)
> +            "")))
> +     ,body))

Good.

>  (defun json-encode-hash-table (hash-table)
>    "Return a JSON representation of HASH-TABLE."
> -  (format "{%s}"
> +  (format (if json-encoding-pretty-print "{%s%s}" "{%s}")

Hmm... if json-encoding-pretty-print is nil, we still pass 2 args, and
the second is always "", so we can always use "{%s%s}", right?

>            (json-join
>             (let (r)
> -             (maphash
> -              (lambda (k v)
> -                (push (format "%s:%s"
> -                              (json-encode-key k)
> -                              (json-encode v))
> -                      r))
> -              hash-table)
> +             (json--with-indentation
> +               (maphash
> +                (lambda (k v)
> +                  (push (format
> +                         (if json-encoding-pretty-print
> +                             "%s%s: %s"
> +                           "%s%s:%s")
> +                         json-encoding-current-indentation
> +                         (json-encode-key k)
> +                         (json-encode v))
> +                        r))
> +                hash-table))
>               r)
> -           ", ")))
> +           json-encoding-separator)
> +          (if json-encoding-lisp-style-closings
> +              ""
> +              json-encoding-current-indentation)))

[ It just occurs to me, that the json printer should print to a buffer,
  rather than to a string.  But let's keep this issue for later.  ]

> -  (concat "[" (mapconcat 'json-encode array ", ") "]"))
> +  (if (and json-encoding-pretty-print
> +           (> (length array) 0))
> +      (concat
> +       (let ((json-encoding-current-indentation
> +              (concat json-encoding-current-indentation
> +                      json-encoding-default-indentation)))

Use json--with-indentation here (even if it performs an extra redundant
test of json-encoding-pretty-print).


        Stefan

Message #44 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Ryan Crum <ryan.crum <at> eleostech.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: Patch for pretty-printing in json.el
Date: Sat, 17 Nov 2012 12:37:17 -0500

[Message part 1 (text/plain, inline)]

Hi Stefan,

I actually have not yet received anything regarding copyright assignment in the mail, so I resubmitted the questionnaire to assign <at> gnu.org today.

New patch attached.

[json-pretty-print.diff (application/octet-stream, attachment)]

[Message part 3 (text/plain, inline)]

On Nov 14, 2012, at 8:56 PM, Stefan Monnier wrote:

> Use a "json--" prefix, since it's a convention we use to express that
> something is internal.

Fixed.

> The first line of a docstring should "stand on it own", i.e. it
> shouldn't end in the middle of a sentence.
> Try C-u M-x checkdoc-current-buffer.

Ah, cool feature. Fixed.

> 
>> (defun json-encode-hash-table (hash-table)
>>   "Return a JSON representation of HASH-TABLE."
>> -  (format "{%s}"
>> +  (format (if json-encoding-pretty-print "{%s%s}" "{%s}")
> 
> Hmm... if json-encoding-pretty-print is nil, we still pass 2 args, and
> the second is always "", so we can always use "{%s%s}", right?

Not exactly, since json--encoding-current-indentation defaults to newline. I've tweaked this part to shift the complexity down into the second clause, however, instead of specifying 2 formats with a potentially unused second argument.

>> -  (concat "[" (mapconcat 'json-encode array ", ") "]"))
>> +  (if (and json-encoding-pretty-print
>> +           (> (length array) 0))
>> +      (concat
>> +       (let ((json-encoding-current-indentation
>> +              (concat json-encoding-current-indentation
>> +                      json-encoding-default-indentation)))
> 
> Use json--with-indentation here (even if it performs an extra redundant
> test of json-encoding-pretty-print).

Agreed, fixed.

I've also added an error handling clause to fix a bug I noticed where if you attempt to `json-pretty-print' some invalid JSON your text gets killed but formatting fails and it doesn't get replaced. Is there a more idiomatic way of doing this? I considered using copy-region-as-kill and then only killing the text if the call to json-encode succeeds, but that seemed more awkward.

Thanks for your help!

-Ryan

Message #47 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Ryan Crum <ryan.crum <at> eleostech.com>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: Patch for pretty-printing in json.el
Date: Tue, 20 Nov 2012 13:52:22 -0500

> I actually have not yet received anything regarding copyright assignment in
> the mail, so I resubmitted the questionnaire to assign <at> gnu.org today.

Hmm... AFAIK nowadays, you should receive the paperwork by email (it
used to be that it had to be printed by the FSF on their own paper, but
last I heard they now just send a PDF which you can print yourself), so
that should be fairly quick.

> New patch attached.

Looks fine, thank you.


        Stefan

Message #50 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Ryan Crum <ryan.crum <at> eleostech.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 12634 <at> debbugs.gnu.org
Subject: Re: bug#12634: Patch for pretty-printing in json.el
Date: Tue, 27 Nov 2012 18:15:49 -0500

Thanks. FYI, I've sent the signed paperwork in.

On Nov 20, 2012, at 1:52 PM, Stefan Monnier wrote:

>> I actually have not yet received anything regarding copyright assignment in
>> the mail, so I resubmitted the questionnaire to assign <at> gnu.org today.
> 
> Hmm... AFAIK nowadays, you should receive the paperwork by email (it
> used to be that it had to be printed by the FSF on their own paper, but
> last I heard they now just send a PDF which you can print yourself), so
> that should be fairly quick.
> 
>> New patch attached.
> 
> Looks fine, thank you.
> 
> 
>        Stefan

Message #53 received at 12634 <at> debbugs.gnu.org (full text, mbox):

From: Ryan Crum <ryan.crum <at> eleostech.com>
To: 12634 <at> debbugs.gnu.org
Subject: Patch
Date: Thu, 13 Dec 2012 21:56:16 -0500

[Message part 1 (text/plain, inline)]

The attached patch applied successfully for me against trunk at rev. 111221.

[json-pretty-print.diff (application/octet-stream, attachment)]

[Message part 3 (text/plain, inline)]

Message #58 received at 12634-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Ryan Crum <ryan.crum <at> eleostech.com>
Cc: 12634-done <at> debbugs.gnu.org
Subject: Re: bug#12634: Patch
Date: Fri, 14 Dec 2012 09:59:07 -0500

> The attached patch applied successfully for me against trunk at rev. 111221.

Thanks, installed, tho with the two commands rewritten (so as to not
affect the kill-ring, and so as to follow the usual convention of
receiving interactive args via the `interactive' spec, and so as to use
atomic-change-group).


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.