GNU bug report logs - #34160
json-pretty-print deletes everything after first JSON object

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 34160 in the body.
You can then email your comments to 34160 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: Albert Heinle <albert.heinle <at> googlemail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: json-pretty-print deletes everything after first JSON object
Date: Mon, 21 Jan 2019 12:46:35 -0500

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

Dear Emacs-dev team,

I have observed the following behavior of the command json-pretty-print
(http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/json.el#n740):

Write

{"a": 1}{"b": 2}

into any Emacs buffer. Then, mark the whole section and run M-x
json-pretty-print.
The line is afterwards altered as
{
  "a": 1
}

This means, that any string after the first completely parsed JSON-object
is being removed by this function. I would consider this unexpected,
because information gets lost after calling a function that is just
supposed to prettify things.

My Emacs version:
GNU Emacs 24.5.1 (x86_64-pc-linux-gnu, GTK+ Version 3.18.9) of 2017-09-20
on lcy01-07, modified by Debian

I also have an Arch-linux system at home where I could also reproduce this
behavior.

Let me know if you have any other questions.

Thank you very much,

Albert Heinle

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

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Albert Heinle <albert.heinle <at> googlemail.com>
Cc: 34160 <at> debbugs.gnu.org
Subject: Re: bug#34160: json-pretty-print deletes everything after first JSON
 object
Date: Tue, 09 Jul 2019 20:41:06 +0200

Albert Heinle <albert.heinle <at> googlemail.com> writes:

> Write
>
> {"a": 1}{"b": 2}
>
> into any Emacs buffer. Then, mark the whole section and run M-x json-pretty-print.
> The line is afterwards altered as
> {
>   "a": 1
> }

I think I've now fixed this on the Emacs trunk.

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

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

From: Glenn Morris <rgm <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Albert Heinle <albert.heinle <at> googlemail.com>, 34160 <at> debbugs.gnu.org
Subject: Re: bug#34160: json-pretty-print deletes everything after first JSON
 object
Date: Wed, 10 Jul 2019 04:53:36 -0400

After the change, test-json-pretty-print-object fails.
Ref eg https://hydra.nixos.org/build/95978732

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Glenn Morris <rgm <at> gnu.org>
Cc: Albert Heinle <albert.heinle <at> googlemail.com>, 34160 <at> debbugs.gnu.org
Subject: Re: bug#34160: json-pretty-print deletes everything after first JSON
 object
Date: Wed, 10 Jul 2019 13:24:06 +0200

Glenn Morris <rgm <at> gnu.org> writes:

> After the change, test-json-pretty-print-object fails.
> Ref eg https://hydra.nixos.org/build/95978732

Thanks; should be fixed now...

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

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

From: Tassilo Horn <tsdh <at> gnu.org>
To: emacs-devel <at> gnu.org
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 34160 <at> debbugs.gnu.org
Subject: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160,
 json.el
Date: Wed, 31 Jul 2019 09:39:00 +0200

Hi Lars,

when fixing bug#34160 you've reverted my changes that made json pretty
printing use replace-region-contents.  That had the major benefit that
pretty printing the JSON object at point didn't move point.  I use that
many times a week on large JSON objects using the following command.

--8<---------------cut here---------------start------------->8---
(defun th/json-pretty-print-snippet-at-point (&optional minimize)
  "Pretty-print the json snippet at point."
  (interactive "P")
  (save-excursion
    (when-let ((beg (car (nth 9 (syntax-ppss)))))
      (goto-char beg)
      (forward-sexp)
      (when (looking-back "\n" beg)
	(backward-char))
      (json-pretty-print beg (point) minimize))))
--8<---------------cut here---------------end--------------->8---

AFAICS, the problem in bug#34160 was not caused by my changes (the user
used Emacs 24 and not a 27 snapshot) so I see no justification for
removing my feature.

Could you please reinstall the feature or describe why it is not
feasible to keep it?

Thanks,
  Tassilo

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tassilo Horn <tsdh <at> gnu.org>
Cc: larsi <at> gnus.org, 34160 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67, 
 bug#34160, json.el
Date: Wed, 31 Jul 2019 18:38:57 +0300

> From: Tassilo Horn <tsdh <at> gnu.org>
> Date: Wed, 31 Jul 2019 09:39:00 +0200
> Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 34160 <at> debbugs.gnu.org
> 
> Hi Lars,
> 
> when fixing bug#34160 you've reverted my changes that made json pretty
> printing use replace-region-contents.  That had the major benefit that
> pretty printing the JSON object at point didn't move point.  I use that
> many times a week on large JSON objects using the following command.
> 
> --8<---------------cut here---------------start------------->8---
> (defun th/json-pretty-print-snippet-at-point (&optional minimize)
>   "Pretty-print the json snippet at point."
>   (interactive "P")
>   (save-excursion
>     (when-let ((beg (car (nth 9 (syntax-ppss)))))
>       (goto-char beg)
>       (forward-sexp)
>       (when (looking-back "\n" beg)
> 	(backward-char))
>       (json-pretty-print beg (point) minimize))))
> --8<---------------cut here---------------end--------------->8---
> 
> AFAICS, the problem in bug#34160 was not caused by my changes (the user
> used Emacs 24 and not a 27 snapshot) so I see no justification for
> removing my feature.
> 
> Could you please reinstall the feature or describe why it is not
> feasible to keep it?

Oops, sorry about that.

I think Lars is on vacation.  If he doesn't respond in a day or two, I
will revert the change until this issue is resolved.

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

From: Tassilo Horn <tsdh <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 34160 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67,
 bug#34160, json.el
Date: Wed, 31 Jul 2019 20:40:00 +0200

Hi chaps,

> I think Lars is on vacation.  If he doesn't respond in a day or two, I
> will revert the change until this issue is resolved.

Thanks, Eli. If nobody is faster, I'll try to fix bug#34160 on the weekend 
in a way that satisfies all of us.

Bye,
 Tassilo

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Tassilo Horn <tsdh <at> gnu.org>
Cc: 34160 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#34160: About commit
 bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el
Date: Wed, 31 Jul 2019 20:41:32 +0200

Tassilo Horn <tsdh <at> gnu.org> writes:

> when fixing bug#34160 you've reverted my changes that made json pretty
> printing use replace-region-contents.  That had the major benefit that
> pretty printing the JSON object at point didn't move point.  I use that
> many times a week on large JSON objects using the following command.

[...]

> AFAICS, the problem in bug#34160 was not caused by my changes (the user
> used Emacs 24 and not a 27 snapshot) so I see no justification for
> removing my feature.

The user referred to
"http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/json.el#n740",
which doesn't look like Emacs 24?

> Could you please reinstall the feature or describe why it is not
> feasible to keep it?

As the bug in question described -- pretty-printing a JSON region would
silently delete everything but the first JSON object, which doesn't seem
like optimal behaviour for a pretty-printing function.

If there's a problem where point is moved unnecessarily, then that
should be fixed, of course.  Do you have a test case?

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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tassilo Horn <tsdh <at> gnu.org>
Cc: larsi <at> gnus.org, 34160 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67,
 bug#34160, json.el
Date: Wed, 31 Jul 2019 21:48:20 +0300

> From: Tassilo Horn <tsdh <at> gnu.org>
> CC: <emacs-devel <at> gnu.org>, <larsi <at> gnus.org>, <34160 <at> debbugs.gnu.org>
> Date: Wed, 31 Jul 2019 20:40:00 +0200
> 
> > I think Lars is on vacation.  If he doesn't respond in a day or two, I
> > will revert the change until this issue is resolved.
> 
> Thanks, Eli. If nobody is faster, I'll try to fix bug#34160 on the weekend 
> in a way that satisfies all of us.

Great, thanks.

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

From: Tassilo Horn <tsdh <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 34160 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#34160: About commit bff64115a0ad081282e0f99305f41c8dd1917d67,
 bug#34160, json.el
Date: Wed, 31 Jul 2019 21:30:12 +0200

> The user referred to
> "http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/json.el#n740",
> which doesn't look like Emacs 24?

Ah, might be.

>> Could you please reinstall the feature or describe why it is not
>> feasible to keep it?
>
> As the bug in question described -- pretty-printing a JSON region would
> silently delete everything but the first JSON object, which doesn't seem
> like optimal behaviour for a pretty-printing function.

Obviously not. :-)

> If there's a problem where point is moved unnecessarily, then that
> should be fixed, of course.  Do you have a test case?

It's not just moving point. replace-region-contents also keeps marks, text 
properties and fontification intact. So we should definitely be using it here.

The loop over all json objects in the region you've added is correct. It's 
just that I beg you to drop the delete-region / insert in favor of 
replace-region-contents.

json-read advances point until the end of the read json. This can be used 
to give the right region (not the complete region as I did) to the repeated 
replace-region-contents calls.

Feel free to give it a try. Otherwise I'll do it on the weekend.

For a test case for point keeping its position in the json: use my command 
from my original mail and an arbitrary json file and invoke it while point 
is somewhere inside the json object.

Bye,
 Tassilo

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Tassilo Horn <tsdh <at> gnu.org>
Cc: 34160 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#34160: About commit
 bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el
Date: Wed, 31 Jul 2019 22:21:10 +0200

Tassilo Horn <tsdh <at> gnu.org> writes:

> It's not just moving point. replace-region-contents also keeps marks,
> text properties and fontification intact. So we should definitely be
> using it here.

Ah, I see.  I only gave that function a cursory look-over when fixing
this bug and I didn't quite understand what it was doing here, since I
couldn't recall any other pretty-printer doing anything similar.

Sorry for the confusion here; I've now restored the
replace-region-contents logic.  Or at least I think so; it works with
the test cases in this bug report, at least.

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

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

From: Tassilo Horn <tsdh <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 34160 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#34160: About commit
 bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el
Date: Thu, 01 Aug 2019 06:54:08 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

Hi Lars,

>> It's not just moving point. replace-region-contents also keeps marks,
>> text properties and fontification intact. So we should definitely be
>> using it here.
>
> Ah, I see.  I only gave that function a cursory look-over when fixing
> this bug and I didn't quite understand what it was doing here, since I
> couldn't recall any other pretty-printer doing anything similar.

No worries!  You probably didn't see it more often because it's quite
new.  But pretty-printing is definitely a very good use-case for it.

> Sorry for the confusion here; I've now restored the
> replace-region-contents logic.  Or at least I think so; it works with
> the test cases in this bug report, at least.

Yes, it works again.  Thanks!

It can still be a bit improved in understandability and efficiency.

1. The function passed to replace-region-contents runs on the narrowed
   buffer anyway, so no need to narrow it yourself.
   
2. It would be better to create a temporary buffer, json-read repeatedly
   from the original buffer, json-encode/insert to the temp one, and
   then return the temp buffer.

The reason for point 2 is that if the function passed to
replace-region-contents returns a string, it'll put that in a temporary
buffer anyhow so that it can use replace-buffer-contents to perform the
actual replacement (replace-region-contents is just a wrapper around
that).

And we might want to cater for the situation where the region starts or
ends inside a json object by copying the buffer substring from (point)
to end to the temp buffer in case json-read fails.  I think right now,
we'd lose such half json objects and everything which follows them.

Bye,
  Tassilo

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Tassilo Horn <tsdh <at> gnu.org>
Cc: 34160 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#34160: About commit
 bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el
Date: Thu, 01 Aug 2019 13:11:24 +0200

Tassilo Horn <tsdh <at> gnu.org> writes:

> It can still be a bit improved in understandability and efficiency.
>
> 1. The function passed to replace-region-contents runs on the narrowed
>    buffer anyway, so no need to narrow it yourself.
>
> 2. It would be better to create a temporary buffer, json-read repeatedly
>    from the original buffer, json-encode/insert to the temp one, and
>    then return the temp buffer.
>
> The reason for point 2 is that if the function passed to
> replace-region-contents returns a string, it'll put that in a temporary
> buffer anyhow so that it can use replace-buffer-contents to perform the
> actual replacement (replace-region-contents is just a wrapper around
> that).

Sounds like a good idea; please go ahead.

> And we might want to cater for the situation where the region starts or
> ends inside a json object by copying the buffer substring from (point)
> to end to the temp buffer in case json-read fails.  I think right now,
> we'd lose such half json objects and everything which follows them.

Yes, that sounds like a good fix.

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

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

From: Tassilo Horn <tsdh <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 34160 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#34160: About commit
 bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el
Date: Thu, 01 Aug 2019 14:16:02 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

>> It can still be a bit improved in understandability and efficiency.
>>
>> 1. The function passed to replace-region-contents runs on the narrowed
>>    buffer anyway, so no need to narrow it yourself.
>>
>> 2. It would be better to create a temporary buffer, json-read repeatedly
>>    from the original buffer, json-encode/insert to the temp one, and
>>    then return the temp buffer.
>>
>> The reason for point 2 is that if the function passed to
>> replace-region-contents returns a string, it'll put that in a temporary
>> buffer anyhow so that it can use replace-buffer-contents to perform the
>> actual replacement (replace-region-contents is just a wrapper around
>> that).
>
> Sounds like a good idea; please go ahead.

Will do at the weekend.

Bye,
  Tassilo

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

From: Tassilo Horn <tsdh <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 34160 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#34160: About commit
 bff64115a0ad081282e0f99305f41c8dd1917d67, bug#34160, json.el
Date: Fri, 02 Aug 2019 18:16:59 +0200

Tassilo Horn <tsdh <at> gnu.org> writes:

Hi Lars,

>>> It can still be a bit improved in understandability and efficiency.
>>>
>>> 1. The function passed to replace-region-contents runs on the narrowed
>>>    buffer anyway, so no need to narrow it yourself.
>>>
>>> 2. It would be better to create a temporary buffer, json-read
>>>    repeatedly from the original buffer, json-encode/insert to the temp
>>>    one, and then return the temp buffer.
>>
>> Sounds like a good idea; please go ahead.
>
> Will do at the weekend.

Ok, I did it just now (10065010a6).  Seems to work very well.

Bye,
Tassilo

Previous Next

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