bug#12634: 24.2; [FEATURE REQUEST] JSON pretty printer

[Leo]

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

[Stefan Monnier]

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

Patch welcome,


Stefan

[Aurélien Aptel]

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

[Stefan Monnier]

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


Stefan

[Aurélien Aptel]

On Fri, Oct 19, 2012 at 3:37 AM, Stefan Monnier
<mon...@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.

[Stefan Monnier]

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

[Stefan Monnier]

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

[Stefan Monnier]

> 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

[Stefan Monnier]

> 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

[Stefan Monnier]

> 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

[Stefan Monnier]

> I actually have not yet received anything regarding copyright assignment in
> the mail, so I resubmitted the questionnaire to ass...@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

[Ryan Crum]

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

[Stefan Monnier]

> 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