[PATCH] Accept plists when serializing and parsing JSON

[PATCH] Accept plists when serializing and parsing JSON

[João Távora]

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

Hi,

So I found this other thread

  https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00667.html

where plist support for json.c is briefly requested and discussed, but
there didn't seem to be an overwhelming argument against it, so I had
another look at the file and it didn't seem very hard or problematic to
implement.

Anyway, I had a go.  It's small, so have a look.  Patch at the end of
this file, or find it in the scratch/support-plists-in-jsonc branch.

I used a global to control json-serialize's interpretation of lists, but
it could be an argument as well.  Added some tests and some doc, too.

João

PS: Take this opportunity to thank Eli and everyone else very much for
the Emacs 26.1 release!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Support-plists-in-json.c.patch --]
[-- Type: text/x-diff, Size: 12594 bytes --]

From 224f8ea95f00cc60ee77aeaad6585bb2ef845f70 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Tue, 29 May 2018 15:41:30 +0100
Subject: [PATCH] Support plists in json.c

* doc/lispref/text.texi (Parsing JSON): Mention plists and
json-serialize-use-plists.

* src/json.c (lisp_to_json_toplevel_1): Decide with
Vjson_serialize_use_plists.
(Fjson_serialize): Update docstring.
(enum json_object_type): Add json_object_plist.
(json_to_lisp): Can build plists.
(json_parse_object_type): Accept plists.
(Fjson_parse_string): Update docstring.
(json-serialize-use-plist): New DEFVAR_LISP.
(Qplist): New sym_of_json

* test/src/json-tests.el (json-serialize/object): Do some tests
with json-serialize-use-plists to t.
(json-parse-string/object): Parse something as a plist.
---
 doc/lispref/text.texi  | 27 ++++++++----
 src/json.c             | 97 ++++++++++++++++++++++++++++++++----------
 test/src/json-tests.el | 18 +++++++-
 3 files changed, 110 insertions(+), 32 deletions(-)

diff --git a/doc/lispref/text.texi b/doc/lispref/text.texi
index da09b4ae1c..3995102237 100644
--- a/doc/lispref/text.texi
+++ b/doc/lispref/text.texi
@@ -5025,16 +5025,18 @@ Parsing JSON
 
 @item
 JSON has only one map type, the object.  JSON objects are represented
-using Lisp hashtables or alists.  When an alist contains several
-elements with the same key, Emacs uses only the first element for
-serialization, in accordance with the behavior of @code{assq}.
+using Lisp hashtables, alists or plists.  When an alist or plist
+contains several elements with the same key, Emacs uses only the first
+element for serialization, in accordance with the behavior of
+@code{assq}.
 
 @end itemize
 
 @noindent
-Note that @code{nil} is a valid alist and represents the empty JSON
-object, @code{@{@}}, not @code{null}, @code{false}, or an empty array,
-all of which are different JSON values.
+Note that @code{nil} is both a valid alist and a valid plist and
+represents the empty JSON object, @code{@{@}}, not @code{null},
+@code{false}, or an empty array, all of which are different JSON
+values.
 
   If some Lisp object can't be represented in JSON, the serialization
 functions will signal an error of type @code{wrong-type-argument}.
@@ -5057,12 +5059,21 @@ Parsing JSON
   Only top-level values (arrays and objects) can be serialized to
 JSON.  The subobjects within these top-level values can be of any
 type.  Likewise, the parsing functions will only return vectors,
-hashtables, and alists.
+hashtables, alists and plists.
 
   The parsing functions accept keyword arguments.  Currently only one
 keyword argument, @code{:object-type}, is recognized; its value can be
 either @code{hash-table} to parse JSON objects as hashtables with
-string keys (the default) or @code{alist} to parse them as alists.
+string keys (the default), @code{alist} to parse them as alists or
+@code{plist} to parse them as plists.
+
+@vindex json-serialize-use-plists
+@cindex serializing plists as json
+  For the serialization function, the variable
+@var{json-serialize-use-plists} controls the converse process,
+resolving the ambiguity when a list is found in the Lisp object to
+serialize. If @code{nil}, its default, the list is interpreted as an
+alist, otherwise it is interpreted as a plist.
 
 @defun json-serialize object
 This function returns a new Lisp string which contains the JSON
diff --git a/src/json.c b/src/json.c
index b046d34f66..ccd58c047e 100644
--- a/src/json.c
+++ b/src/json.c
@@ -395,16 +395,31 @@ lisp_to_json_toplevel_1 (Lisp_Object lisp, json_t **json)
       record_unwind_protect_ptr (json_release_object, *json);
       FOR_EACH_TAIL (tail)
         {
-          Lisp_Object pair = XCAR (tail);
-          CHECK_CONS (pair);
-          Lisp_Object key_symbol = XCAR (pair);
-          Lisp_Object value = XCDR (pair);
+          const char *key_str;
+          Lisp_Object value;
+          Lisp_Object key_symbol;
+          if ( EQ (Vjson_serialize_use_plists, Qt) ) {
+            key_symbol = XCAR (tail);
+            tail = XCDR(tail);
+            CHECK_CONS (tail);
+            value = XCAR (tail);
+            if (EQ (tail, li.tortoise)) circular_list (lisp);
+          } else {
+            Lisp_Object pair = XCAR (tail);
+            CHECK_CONS (pair);
+            key_symbol = XCAR (pair);
+            value = XCDR (pair);
+          }
           CHECK_SYMBOL (key_symbol);
           Lisp_Object key = SYMBOL_NAME (key_symbol);
           /* We can't specify the length, so the string must be
              null-terminated.  */
           check_string_without_embedded_nulls (key);
-          const char *key_str = SSDATA (key);
+          key_str = SSDATA (key);
+          /* If using plists, maybe strip the ":" from symbol-name */
+          if (EQ (Vjson_serialize_use_plists, Qt) &&
+              ':' == key_str[0] &&
+              key_str[1] ) key_str = &key_str[1];
           /* Only add element if key is not already present.  */
           if (json_object_get (*json, key_str) == NULL)
             {
@@ -476,13 +491,17 @@ lisp_to_json (Lisp_Object lisp)
 
 DEFUN ("json-serialize", Fjson_serialize, Sjson_serialize, 1, 1, NULL,
        doc: /* Return the JSON representation of OBJECT as a string.
-OBJECT must be a vector, hashtable, or alist, and its elements can
-recursively contain `:null', `:false', t, numbers, strings, or other
-vectors hashtables, and alist.  `:null', `:false', and t will be
-converted to JSON null, false, and true values, respectively.  Vectors
-will be converted to JSON arrays, and hashtables and alists to JSON
-objects.  Hashtable keys must be strings without embedded null
-characters and must be unique within each object.  Alist keys must be
+
+OBJECT must be a vector of values or a key-value map.  Hashtables,
+alists and plists are accepted as maps, the variable
+`json-serialize-use-plists' controlling which one of the latter two to
+use.  In any of these cases, values can be `:null', `:false', t,
+numbers, strings, or, recursively, other vectors, hashtables, alists
+or plists.  `:null', `:false', and t will be converted to JSON null,
+false, and true values, respectively.  Vectors will be converted to
+JSON arrays, and hashtables, alists and plists to JSON objects.
+Hashtable keys must be strings without embedded null characters and
+must be unique within each object.  Alist or plist keys must be
 symbols; if a key is duplicate, the first instance is used.  */)
   (Lisp_Object object)
 {
@@ -605,6 +624,7 @@ OBJECT.  */)
 enum json_object_type {
   json_object_hashtable,
   json_object_alist,
+  json_object_plist
 };
 
 /* Convert a JSON object to a Lisp object.  */
@@ -692,6 +712,30 @@ json_to_lisp (json_t *json, enum json_object_type object_type)
               result = Fnreverse (result);
               break;
             }
+          case json_object_plist:
+            {
+              result = Qnil;
+              const char *key_str;
+              json_t *value;
+              json_object_foreach (json, key_str, value)
+                {
+                  /* No idea if using AUTO_STRING and Fconcat for
+                     making keywords is idiomatic, but seems to work
+                     nicely */
+                  AUTO_STRING (colon, ":");
+                  Lisp_Object key =
+                    Fintern (CALLN (Fconcat, colon, json_build_string (key_str))
+                             , Qnil);
+                  result = Fcons (key, result); /* build the plist as
+                                                   value-key since
+                                                   we're going to
+                                                   reverse it in the
+                                                   end.*/
+                  result = Fcons (json_to_lisp (value, object_type), result);
+                }
+              result = Fnreverse (result);
+              break;
+            }
           default:
             /* Can't get here.  */
             emacs_abort ();
@@ -721,8 +765,10 @@ json_parse_object_type (ptrdiff_t nargs, Lisp_Object *args)
           return json_object_hashtable;
         else if (EQ (value, Qalist))
           return json_object_alist;
+        else if (EQ (value, Qplist))
+          return json_object_plist;
         else
-          wrong_choice (list2 (Qhash_table, Qalist), value);
+          wrong_choice (list3 (Qhash_table, Qalist, Qplist), value);
       }
     default:
       wrong_type_argument (Qplistp, Flist (nargs, args));
@@ -733,15 +779,16 @@ DEFUN ("json-parse-string", Fjson_parse_string, Sjson_parse_string, 1, MANY,
        NULL,
        doc: /* Parse the JSON STRING into a Lisp object.
 This is essentially the reverse operation of `json-serialize', which
-see.  The returned object will be a vector, hashtable, or alist.  Its
-elements will be `:null', `:false', t, numbers, strings, or further
-vectors, hashtables, and alists.  If there are duplicate keys in an
-object, all but the last one are ignored.  If STRING doesn't contain a
-valid JSON object, an error of type `json-parse-error' is signaled.
-The keyword argument `:object-type' specifies which Lisp type is used
-to represent objects; it can be `hash-table' or `alist'.
-usage: (json-parse-string STRING &key (OBJECT-TYPE \\='hash-table))  */)
-  (ptrdiff_t nargs, Lisp_Object *args)
+see.  The returned object will be a vector, hashtable, alist, or
+plist.  Its elements will be `:null', `:false', t, numbers, strings,
+or further vectors, hashtables, alists, or plists.  If there are
+duplicate keys in an object, all but the last one are ignored.  If
+STRING doesn't contain a valid JSON object, an error of type
+`json-parse-error' is signaled.  The keyword argument `:object-type'
+specifies which Lisp type is used to represent objects; it can be
+`hash-table', `alist' or `plist'.
+usage: (json-parse-string STRING &key (OBJECT-TYPE \\='hash-table)) */)
+     (ptrdiff_t nargs, Lisp_Object *args)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
 
@@ -903,6 +950,11 @@ syms_of_json (void)
   DEFSYM (Qpure, "pure");
   DEFSYM (Qside_effect_free, "side-effect-free");
 
+  DEFVAR_LISP ("json-serialize-use-plists", Vjson_serialize_use_plists,
+               doc:
+               /* If non-nil use plists instead of alists in json-serialize.*/);
+  Vjson_serialize_use_plists = Qnil;
+
   DEFSYM (Qjson_serialize, "json-serialize");
   DEFSYM (Qjson_parse_string, "json-parse-string");
   Fput (Qjson_serialize, Qpure, Qt);
@@ -912,6 +964,7 @@ syms_of_json (void)
 
   DEFSYM (QCobject_type, ":object-type");
   DEFSYM (Qalist, "alist");
+  DEFSYM (Qplist, "plist");
 
   defsubr (&Sjson_serialize);
   defsubr (&Sjson_insert);
diff --git a/test/src/json-tests.el b/test/src/json-tests.el
index 09067bad8c..5c9be20e95 100644
--- a/test/src/json-tests.el
+++ b/test/src/json-tests.el
@@ -69,7 +69,19 @@ 'json-tests--error
   (should-error (json-serialize '((1 . 2))) :type 'wrong-type-argument)
   (should-error (json-serialize '((a . 1) . b)) :type 'wrong-type-argument)
   (should-error (json-serialize '#1=((a . 1) . #1#)) :type 'circular-list)
-  (should-error (json-serialize '(#1=(a #1#)))))
+  (should-error (json-serialize '(#1=(a #1#))))
+
+  (let ((json-serialize-use-plists t))
+    (should (equal (json-serialize '(:abc [1 2 t] :def :null))
+                   "{\"abc\":[1,2,true],\"def\":null}"))
+    (should (equal (json-serialize '(abc [1 2 t] :def :null))
+                   "{\"abc\":[1,2,true],\"def\":null}"))
+    (should-error (json-serialize '#1=(:a 1 . #1#)) :type 'circular-list)
+    (should-error (json-serialize '((abc . 1))) :type 'wrong-type-argument)
+    (should-error (json-serialize '(:foo bar (abc . 1)))
+                  :type 'wrong-type-argument)
+    (should-error (json-serialize '(:foo bar :odd-numbered))
+                  :type 'wrong-type-argument)))
 
 (ert-deftest json-serialize/object-with-duplicate-keys ()
   (skip-unless (fboundp 'json-serialize))
@@ -89,7 +101,9 @@ 'json-tests--error
       (should (equal (cl-sort (map-pairs actual) #'string< :key #'car)
                      '(("abc" . [9 :false]) ("def" . :null)))))
     (should (equal (json-parse-string input :object-type 'alist)
-                   '((abc . [9 :false]) (def . :null))))))
+                   '((abc . [9 :false]) (def . :null))))
+    (should (equal (json-parse-string input :object-type 'plist)
+                   '(:abc [9 :false] :def :null)))))
 
 (ert-deftest json-parse-string/string ()
   (skip-unless (fboundp 'json-parse-string))
-- 
2.17.0

Re: [PATCH] Accept plists when serializing and parsing JSON

[Philipp Stephani]

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

João Távora <joaotavora@gmail.com> schrieb am Di., 29. Mai 2018 um
16:59 Uhr:

>
> I used a global to control json-serialize's interpretation of lists, but
> it could be an argument as well.


Please don't add global state to json.c. It's a low-level library that
shouldn't rely on global state (and global state should generally be
avoided in libraries). Not having global state was one of my explicit
design goals for json.c

[-- Attachment #2: Type: text/html, Size: 723 bytes --]

Re: [PATCH] Accept plists when serializing and parsing JSON

[João Távora]

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

On Tue, May 29, 2018, 22:21 Philipp Stephani <p.stephani2@gmail.com> wrote:

>
>
> João Távora <joaotavora@gmail.com> schrieb am Di., 29. Mai 2018 um
> 16:59 Uhr:
>
>>
>> I used a global to control json-serialize's interpretation of lists, but
>> it could be an argument as well.
>
>
> Please don't add global state to json.c. It's a low-level library that
> shouldn't rely on global state (and global state should generally be
> avoided in libraries). Not having global state was one of my explicit
> design goals for json.c
>

Well, it's not really "global state" in the sense I believe you're talking
about, because no part of the library is maintaining any state in global
variables -- it's read-only from json.c.

That said, if that's your only objection, consider that duck removed :)

João

[-- Attachment #2: Type: text/html, Size: 1742 bytes --]

Re: [PATCH] Accept plists when serializing and parsing JSON

[Yuri Khan]

On Wed, May 30, 2018 at 5:31 AM João Távora <joaotavora@gmail.com> wrote:

> On Tue, May 29, 2018, 22:21 Philipp Stephani <p.stephani2@gmail.com>
wrote:

>> Please don't add global state to json.c. It's a low-level library that
shouldn't rely on global state (and global state should generally be
avoided in libraries). Not having global state was one of my explicit
design goals for json.c

> Well, it's not really "global state" in the sense I believe you're
talking about, because no part of the library is maintaining any state in
global variables -- it's read-only from json.c.

I think the objection to global state (or a global setting, as the case may
be) can be illustrated with the following scenario.

* Consider two independent Elisp packages ‘foo’ and ‘bar’, both of which
want to use the JSON library.
* ‘foo’ wants plists while ‘bar’ wants alists.
* ‘foo’ has a hook. The user adds to that hook a function that invokes
‘bar’.
* Result (after ironing out all bugs related to shared state): Every call
to the JSON library has to be wrapped in a (let ((json-serialize-use-plists
…)) …), which is more cumbersome than just passing an argument to a
function.

Re: [PATCH] Accept plists when serializing and parsing JSON

[João Távora]

Yuri Khan <yurivkhan@gmail.com> writes:

> On Wed, May 30, 2018 at 5:31 AM João Távora <joaotavora@gmail.com> wrote:
>
>> Well, it's not really "global state" in the sense I believe you're
>> talking about, because no part of the library is maintaining any state in
>> global variables -- it's read-only from json.c.

> after ironing out all [the other?] bugs related to shared state

Which ones? Those seem to be precisely the ones I meant to say are
excluded because the var is read-only.

> * ‘foo’ wants plists while ‘bar’ wants alists.

If "want" is the same thing as "must have", then bar should be setting
the var from the beginning, end of story. It should only not set the var
if it doesn't care.

There are many variables like this in emacs. Eli recently reminded me in
a closely related thread I have to bind coding-system-for-write around
process-send-string, for example.

Obviously, the pitfall is overlooking to bind them when you have
to. Presumably this is outweighed by the convenience of not passing a
trillion args to a function call and, more importantly, to every caller
of a function, like in the commit that removes the var, which is quite a
bit more complex (which is why I started with the other alternative).

Anyway, this point could be irrelevant if we remove the variable and
argument completely, auto-detecting plists. Can't decide if that could
be a worse idea, though.

João

Re: [PATCH] Accept plists when serializing and parsing JSON

[Philipp Stephani]

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

João Távora <joaotavora@gmail.com> schrieb am Mi., 30. Mai 2018 um
10:58 Uhr:

> Yuri Khan <yurivkhan@gmail.com> writes:
>
> > On Wed, May 30, 2018 at 5:31 AM João Távora <joaotavora@gmail.com>
> wrote:
> >
> >> Well, it's not really "global state" in the sense I believe you're
> >> talking about, because no part of the library is maintaining any state
> in
> >> global variables -- it's read-only from json.c.
>
> > after ironing out all [the other?] bugs related to shared state
>
> Which ones? Those seem to be precisely the ones I meant to say are
> excluded because the var is read-only.
>

It is not read-only. If it were read-only, it would be just a constant and
not a variable (in the imperative programming sense) at all.


>
> > * ‘foo’ wants plists while ‘bar’ wants alists.
>
> If "want" is the same thing as "must have", then bar should be setting
> the var from the beginning, end of story. It should only not set the var
> if it doesn't care.
>

(Almost) every caller cares about the return type of the function call;
what else should it care about? That means that every caller would have to
set this variable, turning it into a parameter, just with awkward syntax
and subtle, easy-to-get-wrong interface.


>
> There are many variables like this in emacs.


Yes, unfortunately. That's no reason to introduce new ones.
Emacs was written in simpler times (the 1970s?) when the amount of code in
the world was much smaller and people didn't yet know everything that we
now know about software engineering. That's OK; if you look at Fortran code
from that era it also looks very different from what people would write
today (six-letter function names, a single static callback function,
customization happens generally by copying and modifying functions, ...).
But we have 2018, so we should apply the accumulated wisdom from the last
decades.


> Eli recently reminded me in
> a closely related thread I have to bind coding-system-for-write around
> process-send-string, for example.
>

Yes, that's necessary because the existing interface of process-send-string
uses dynamic variables as hidden parameters. That's unfortunate, but no
reason why other, unrelated APIs should also do that.


>
> Obviously, the pitfall is overlooking to bind them when you have
> to. Presumably this is outweighed by the convenience of not passing a
> trillion args to a function call and, more importantly, to every caller
> of a function, like in the commit that removes the var, which is quite a
> bit more complex (which is why I started with the other alternative).
>

No, it's never outweighted. If there are too many parameters, you'd
collecting them in an "options" object and pass that around, or create a
"JsonDecoder" object that keeps the settings, etc.


>
> Anyway, this point could be irrelevant if we remove the variable and
> argument completely, auto-detecting plists.

Can't decide if that could
> be a worse idea, though.
>
>
It might be because it could make the interface more surprising. There's a
clear type difference between hashtables and lists, so detecting them
automatically is fine. But "plist" isn't really a type; there's no
fundamental difference between plists and alists, and the only difference
is how they are used (assq vs. plist-get etc.). That's one of the (several)
reasons why I decided to not support plists in json.c at all, and I'm still
not convinced it's worth the additional interface complexity (for
deserializing the story is probably different, as it doesn't make the
existing interface more complex, and with the CL argument lists you already
have an actual use case).

[-- Attachment #2: Type: text/html, Size: 5162 bytes --]

Re: [PATCH] Accept plists when serializing and parsing JSON

[João Távora]

Philipp Stephani <p.stephani2@gmail.com> writes:

> (a 1 b 2) gets serialized to {"a": 1, "b": 2} and then might reasonably
> conclude that (a 1 :a 2) gets serialized to {"a": 1, ":a": 2}, but that
> would be wrong. It's more obvious and easier to understand to not treat
> colons specially.

I think we should do what json.el does here. It's doing otherwise that
would confuse people.

>>   char *keyword_key_str = SAFE_ALLOCA (1 + strlen(key_str) + 1);
>>   keyword_key_str[0]=':';
> Again, no special treatment for colons please.

No. Because this would, with no obvious reason , defeat the main purpose
of making plists, which is to use them in destructuring lambda lists,
such as the one you yourself chose for json-parse-string.

Regarding the doc comments, I'll do my best to rephrase and use
consistent commas. I'll also add the test you suggested as well.

>>>> talking about, because no part of the library is maintaining any state in
>>>> global variables -- it's read-only from json.c.
>>> after ironing out all [the other?] bugs related to shared state
>>  Which ones? Those seem to be precisely the ones I meant to say are
>>  excluded because the var is read-only.
> It is not read-only. If it were read-only, it would be just a constant
> and not a variable (in the imperative programming sense) at all.

Read some 5 lines above and you will see that what I meant, and indeed
wrote, it's "read-only from json.c"". You may hold another
interpretation, and that's just fine, but by "global state", in Emacs, I
understand things like buffer, point, mark, match data, etc... That is,
I mean exactly what Emacs adds to these functions' docstrings. From
help-fns.el

  (when (or (function-get function 'pure)
            (function-get function 'side-effect-free))
          (insert "\nThis function does not change global state, "
                  "including the match data."))

This is what I mean by global state. Anything else is potentially
erudite bikeshedding in which I'm not particularly interested right now.

>  There are many variables like this in emacs.
>
> Yes, unfortunately. That's no reason to introduce new ones.  Emacs was
> written in simpler times (the 1970s?) when the amount of code in the
> world was much smaller and people didn't yet know everything that we

I really don't think of programmers of the 70's as these simpletons,
quite the contrary actually. Certainly the best programming paradigms
IMHO are those started around that time, in Common Lisp for example,
where dynamic variables abound. But as I write this, I feel the
trappings another bikeshed starting to materialize so I must say that's
just, like, my opinion, man.

João

Re: [PATCH] Accept plists when serializing and parsing JSON

[Stefan Monnier]

> wrote, it's "read-only from json.c"". You may hold another
> interpretation, and that's just fine, but by "global state", in Emacs, I
> understand things like buffer, point, mark, match data, etc... That is,
> I mean exactly what Emacs adds to these functions' docstrings. From
> help-fns.el
>
>   (when (or (function-get function 'pure)
>             (function-get function 'side-effect-free))
>           (insert "\nThis function does not change global state, "
>                   "including the match data."))
>
> This is what I mean by global state. Anything else is potentially
> erudite bikeshedding in which I'm not particularly interested right now.

Notice the use of "change" in that docstring.  Looking up the value of
a dynamically-scoped variable doesn't *change* global state, but it
*uses* global state.

But yes, this is quickly falling into "erudite bikeshedding" territory,
so I'll stop before giving my opinion.


        Stefan

Re: [PATCH] Accept plists when serializing and parsing JSON

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Notice the use of "change" in that docstring.  Looking up the value of
> a dynamically-scoped variable doesn't *change* global state, but it
> *uses* global state.

Yeah, but it's the "change" bit that usually counts: it's (sneakily)
writing shared state that has the bug potential alluded to by Yuri.

That said, lisp is particularly bad territory for purity advocates. Many
many functions use global state, and many more change it just by consing
up something, or interning a symbol.

Re: [PATCH] Accept plists when serializing and parsing JSON

[Philipp Stephani]

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

Yuri Khan <yurivkhan@gmail.com> schrieb am Mi., 30. Mai 2018 um 08:37 Uhr:

> On Wed, May 30, 2018 at 5:31 AM João Távora <joaotavora@gmail.com> wrote:
>
> > On Tue, May 29, 2018, 22:21 Philipp Stephani <p.stephani2@gmail.com>
> wrote:
>
> >> Please don't add global state to json.c. It's a low-level library that
> shouldn't rely on global state (and global state should generally be
> avoided in libraries). Not having global state was one of my explicit
> design goals for json.c
>
> > Well, it's not really "global state" in the sense I believe you're
> talking about, because no part of the library is maintaining any state in
> global variables -- it's read-only from json.c.
>
> I think the objection to global state (or a global setting, as the case may
> be) can be illustrated with the following scenario.
>
> * Consider two independent Elisp packages ‘foo’ and ‘bar’, both of which
> want to use the JSON library.
> * ‘foo’ wants plists while ‘bar’ wants alists.
> * ‘foo’ has a hook. The user adds to that hook a function that invokes
> ‘bar’.
> * Result (after ironing out all bugs related to shared state): Every call
> to the JSON library has to be wrapped in a (let ((json-serialize-use-plists
> …)) …), which is more cumbersome than just passing an argument to a
> function.
>

Yes, exactly. Any dynamic variable is effectively a hidden parameter.
That's much worse than an explicit parameter because you have to remember
that it's a parameter and explicitly bind it on every call.
Consider the Java equivalent:

class Json {
  public static Object deserialize(String json) { ... }  // uses
useHashtables
  public static bool useHashtables = false;
}

Very few people would consider such an interface good style: there's a
subtle interaction between deserialize and useHashtables, and all callers
need to be aware of the interaction. But interfaces should be easy to use
correctly and hard to use incorrectly, and this interface is easy to use
incorrectly and hard to use correctly. BTW, This has nothing to do with
Java or ELisp, but is generally true for all (imperative) programming
languages: avoid mutable global state, avoid "action at a distance"
(seemingly unrelated parts of the code influencing each other). For ELisp,
this means: avoid introducing dynamic variables if possible.

[-- Attachment #2: Type: text/html, Size: 2976 bytes --]

Re: [PATCH] Accept plists when serializing and parsing JSON

[Philipp Stephani]

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

João Távora <joaotavora@gmail.com> schrieb am Mi., 30. Mai 2018 um
00:31 Uhr:

> On Tue, May 29, 2018, 22:21 Philipp Stephani <p.stephani2@gmail.com>
> wrote:
>
>>
>>
>> João Távora <joaotavora@gmail.com> schrieb am Di., 29. Mai 2018 um
>> 16:59 Uhr:
>>
>>>
>>> I used a global to control json-serialize's interpretation of lists, but
>>> it could be an argument as well.
>>
>>
>> Please don't add global state to json.c. It's a low-level library that
>> shouldn't rely on global state (and global state should generally be
>> avoided in libraries). Not having global state was one of my explicit
>> design goals for json.c
>>
>
> Well, it's not really "global state" in the sense I believe you're talking
> about, because no part of the library is maintaining any state in global
> variables -- it's read-only from json.c.
>

Every defvar, defcustom, defconst is global mutable state.


>
> That said, if that's your only objection, consider that duck removed :)
>
>
It's not my only objection, I just didn't have time yet to review the rest
of the code, sorry.

[-- Attachment #2: Type: text/html, Size: 2267 bytes --]

Re: [PATCH] Accept plists when serializing and parsing JSON

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Tue, 29 May 2018 15:59:06 +0100
> 
> So I found this other thread
> 
>   https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00667.html
> 
> where plist support for json.c is briefly requested and discussed, but
> there didn't seem to be an overwhelming argument against it, so I had
> another look at the file and it didn't seem very hard or problematic to
> implement.
> 
> Anyway, I had a go.

Thanks, please see a few comments below.

> +@vindex json-serialize-use-plists
> +@cindex serializing plists as json
> +  For the serialization function, the variable
> +@var{json-serialize-use-plists} controls the converse process,

json-serialize-use-plists is the literal name of a variable, so it
should have the @code markup, not @var.

> +resolving the ambiguity when a list is found in the Lisp object to
> +serialize. If @code{nil}, its default, the list is interpreted as an
            ^^
Two spaces between sentences, please.

> +          if ( EQ (Vjson_serialize_use_plists, Qt) ) {
> +            key_symbol = XCAR (tail);
> +            tail = XCDR(tail);
> +            CHECK_CONS (tail);
> +            value = XCAR (tail);
> +            if (EQ (tail, li.tortoise)) circular_list (lisp);
> +          } else {
> +            Lisp_Object pair = XCAR (tail);

Our style is to write

  if (something)
    {
       then-part;
    }
  else
    {
       else-part;
    }

Also, please don't leav any whitespace between the opening parenthesis
and the following character, and similarly with closing parens.  IOW,
this:

   if ( EQ (Vjson_serialize_use_plists, Qt) )

should be this instead:

   if (EQ (Vjson_serialize_use_plists, Qt))

> +              json_object_foreach (json, key_str, value)
> +                {
> +                  /* No idea if using AUTO_STRING and Fconcat for
> +                     making keywords is idiomatic, but seems to work
> +                     nicely */
> +                  AUTO_STRING (colon, ":");
> +                  Lisp_Object key =
> +                    Fintern (CALLN (Fconcat, colon, json_build_string (key_str))
> +                             , Qnil);
> +                  result = Fcons (key, result); /* build the plist as
> +                                                   value-key since
> +                                                   we're going to
> +                                                   reverse it in the
> +                                                   end.*/
> +                  result = Fcons (json_to_lisp (value, object_type), result);
> +                }

Here I'd use intern_1 instead, it would allow you to avoid
unnecessarily consing Lisp objects.  (Yes, I realize that the same
comment applies to the existing code.)

> +               /* If non-nil use plists instead of alists in json-serialize.*/);
                               ^
Comma is missing there.

Re: [PATCH] Accept plists when serializing and parsing JSON

[João Távora]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, please see a few comments below.

Thanks for reviewing.

> json-serialize-use-plists is the literal name of a variable, so it
> should have the @code markup, not @var.
>
Thanks for these doc comments, but I've gotten rid of
json-serialize-use-plists, after coming to the conclusion that the map
type (hashtable, alist, or plist) can be reliably auto-detected in
json-serialize.  As a consequence, less stuff to document..


>> +          if ( EQ (Vjson_serialize_use_plists, Qt) ) {
>> +            key_symbol = XCAR (tail);
>> +            tail = XCDR(tail);
>> +            CHECK_CONS (tail);
>> +            value = XCAR (tail);
>> +            if (EQ (tail, li.tortoise)) circular_list (lisp);
>> +          } else {
>> +            Lisp_Object pair = XCAR (tail);
>
> Our style is to write
>
> [...]
>
> Also, please don't leav any whitespace between the opening parenthesis
> and the following character, and similarly with closing parens.  IOW,
> this:

OK, got it.

> Here I'd use intern_1 instead, it would allow you to avoid
> unnecessarily consing Lisp objects.  (Yes, I realize that the same
> comment applies to the existing code.)

Riight, intern_1 sounds good... I know I can't just malloc() stuff as
usually right? But also, I have no idea what I'm doing, I aped this from
somewhere where it looked more-or-less responsibly done.

  json_object_foreach (json, key_str, value)
  {
    USE_SAFE_ALLOCA;
    char *keyword_key_str = SAFE_ALLOCA (1 + strlen(key_str) + 1);
    keyword_key_str[0]=':';
    strcpy(&keyword_key_str[1],key_str);
    Lisp_Object key = intern_1(keyword_key_str, strlen(key_str)+1);
    result = Fcons (key, result); /* build the plist as
                                     value-key since
                                     we're going to
                                     reverse it in the
                                     end.*/
    result = Fcons (json_to_lisp (value, object_type), result);
    SAFE_FREE ();
  }

New patch after my sig
João

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Accept-plists-when-serializing-and-parsing-JSON.patch --]
[-- Type: text/x-diff, Size: 12332 bytes --]

From 33fdc8ccf903412b47c2fa61e2ebe5f67245df73 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Sat, 2 Jun 2018 00:23:38 +0100
Subject: [PATCH] Accept plists when serializing and parsing JSON

* doc/lispref/text.texi (Parsing JSON): Mention plist support.

* src/json.c (lisp_to_json_toplevel_1): Serialize plists to json.
(Fjson_serialize): Mention plists in docstring.
(enum json_object_type): Add json_object_plist
(json_to_lisp): Parse JSON into plists.
(json_parse_object_type): Consider plists.
(Fjson_parse_string): Mention plists in docstring.
(syms_of_json): New Qplist sym_of_json

* test/src/json-tests.el (json-serialize/object)
(json-parse-string/object): New plist tests.
---
 doc/lispref/text.texi  | 25 ++++++-----
 src/json.c             | 94 +++++++++++++++++++++++++++++++-----------
 test/src/json-tests.el | 29 ++++++++++++-
 3 files changed, 113 insertions(+), 35 deletions(-)

diff --git a/doc/lispref/text.texi b/doc/lispref/text.texi
index da09b4ae1c..7ce79bbbea 100644
--- a/doc/lispref/text.texi
+++ b/doc/lispref/text.texi
@@ -5025,16 +5025,18 @@ Parsing JSON
 
 @item
 JSON has only one map type, the object.  JSON objects are represented
-using Lisp hashtables or alists.  When an alist contains several
-elements with the same key, Emacs uses only the first element for
-serialization, in accordance with the behavior of @code{assq}.
+using Lisp hashtables, alists or plists.  When an alist or plist
+contains several elements with the same key, Emacs uses only the first
+element for serialization, in accordance with the behavior of
+@code{assq}.
 
 @end itemize
 
 @noindent
-Note that @code{nil} is a valid alist and represents the empty JSON
-object, @code{@{@}}, not @code{null}, @code{false}, or an empty array,
-all of which are different JSON values.
+Note that @code{nil} is both a valid alist and a valid plist and
+represents the empty JSON object, @code{@{@}}, not @code{null},
+@code{false}, or an empty array, all of which are different JSON
+values.
 
   If some Lisp object can't be represented in JSON, the serialization
 functions will signal an error of type @code{wrong-type-argument}.
@@ -5057,12 +5059,15 @@ Parsing JSON
   Only top-level values (arrays and objects) can be serialized to
 JSON.  The subobjects within these top-level values can be of any
 type.  Likewise, the parsing functions will only return vectors,
-hashtables, and alists.
+hashtables, alists and plists.
 
   The parsing functions accept keyword arguments.  Currently only one
-keyword argument, @code{:object-type}, is recognized; its value can be
-either @code{hash-table} to parse JSON objects as hashtables with
-string keys (the default) or @code{alist} to parse them as alists.
+keyword argument, @code{:object-type}, is recognized; its value
+decides which Lisp object to use for representing the key-value
+mappings of a JSON object.  It can be either @code{hash-table}, the
+default, to make hashtables with strings as keys, @code{alist} to use
+alists with symbols as keys or @code{plist} to use plists with keyword
+symbols as keys.
 
 @defun json-serialize object
 This function returns a new Lisp string which contains the JSON
diff --git a/src/json.c b/src/json.c
index b046d34f66..69c689c97c 100644
--- a/src/json.c
+++ b/src/json.c
@@ -393,18 +393,37 @@ lisp_to_json_toplevel_1 (Lisp_Object lisp, json_t **json)
       *json = json_check (json_object ());
       ptrdiff_t count = SPECPDL_INDEX ();
       record_unwind_protect_ptr (json_release_object, *json);
+      bool is_plist=!CONSP(XCAR (tail));
       FOR_EACH_TAIL (tail)
         {
-          Lisp_Object pair = XCAR (tail);
-          CHECK_CONS (pair);
-          Lisp_Object key_symbol = XCAR (pair);
-          Lisp_Object value = XCDR (pair);
+          const char *key_str;
+          Lisp_Object value;
+          Lisp_Object key_symbol;
+          if (is_plist)
+            {
+              key_symbol = XCAR (tail);
+              tail = XCDR(tail);
+              CHECK_CONS (tail);
+              value = XCAR (tail);
+              if (EQ (tail, li.tortoise)) circular_list (lisp);
+            }
+          else
+            {
+              Lisp_Object pair = XCAR (tail);
+              CHECK_CONS (pair);
+              key_symbol = XCAR (pair);
+              value = XCDR (pair);
+            }
           CHECK_SYMBOL (key_symbol);
           Lisp_Object key = SYMBOL_NAME (key_symbol);
           /* We can't specify the length, so the string must be
              null-terminated.  */
           check_string_without_embedded_nulls (key);
-          const char *key_str = SSDATA (key);
+          key_str = SSDATA (key);
+          /* If using plists, maybe strip the ":" from symbol-name */
+          if (is_plist &&
+              ':' == key_str[0] &&
+              key_str[1] ) key_str = &key_str[1];
           /* Only add element if key is not already present.  */
           if (json_object_get (*json, key_str) == NULL)
             {
@@ -476,14 +495,15 @@ lisp_to_json (Lisp_Object lisp)
 
 DEFUN ("json-serialize", Fjson_serialize, Sjson_serialize, 1, 1, NULL,
        doc: /* Return the JSON representation of OBJECT as a string.
-OBJECT must be a vector, hashtable, or alist, and its elements can
-recursively contain `:null', `:false', t, numbers, strings, or other
-vectors hashtables, and alist.  `:null', `:false', and t will be
-converted to JSON null, false, and true values, respectively.  Vectors
-will be converted to JSON arrays, and hashtables and alists to JSON
-objects.  Hashtable keys must be strings without embedded null
-characters and must be unique within each object.  Alist keys must be
-symbols; if a key is duplicate, the first instance is used.  */)
+OBJECT must be a vector, hashtable, alist, or plist and its elements
+can recursively contain `:null', `:false', t, numbers, strings, or
+other vectors hashtables, alists or plists.  `:null', `:false', and t
+will be converted to JSON null, false, and true values, respectively.
+Vectors will be converted to JSON arrays, and hashtables and alists to
+JSON objects.  Hashtable keys must be strings without embedded null
+characters and must be unique within each object.  Alist and plist
+keys must be symbols; if a key is duplicate, the first instance is
+used.  */)
   (Lisp_Object object)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
@@ -605,6 +625,7 @@ OBJECT.  */)
 enum json_object_type {
   json_object_hashtable,
   json_object_alist,
+  json_object_plist
 };
 
 /* Convert a JSON object to a Lisp object.  */
@@ -692,6 +713,29 @@ json_to_lisp (json_t *json, enum json_object_type object_type)
               result = Fnreverse (result);
               break;
             }
+          case json_object_plist:
+            {
+              result = Qnil;
+              const char *key_str;
+              json_t *value;
+              json_object_foreach (json, key_str, value)
+                {
+                  USE_SAFE_ALLOCA;
+                  char *keyword_key_str = SAFE_ALLOCA (1 + strlen(key_str) + 1);
+                  keyword_key_str[0]=':';
+                  strcpy(&keyword_key_str[1],key_str);
+                  Lisp_Object key = intern_1(keyword_key_str,strlen(key_str)+1);
+                  result = Fcons (key, result); /* build the plist as
+                                                   value-key since
+                                                   we're going to
+                                                   reverse it in the
+                                                   end.*/
+                  result = Fcons (json_to_lisp (value, object_type), result);
+                  SAFE_FREE ();
+                }
+              result = Fnreverse (result);
+              break;
+            }
           default:
             /* Can't get here.  */
             emacs_abort ();
@@ -721,8 +765,10 @@ json_parse_object_type (ptrdiff_t nargs, Lisp_Object *args)
           return json_object_hashtable;
         else if (EQ (value, Qalist))
           return json_object_alist;
+        else if (EQ (value, Qplist))
+          return json_object_plist;
         else
-          wrong_choice (list2 (Qhash_table, Qalist), value);
+          wrong_choice (list3 (Qhash_table, Qalist, Qplist), value);
       }
     default:
       wrong_type_argument (Qplistp, Flist (nargs, args));
@@ -733,15 +779,16 @@ DEFUN ("json-parse-string", Fjson_parse_string, Sjson_parse_string, 1, MANY,
        NULL,
        doc: /* Parse the JSON STRING into a Lisp object.
 This is essentially the reverse operation of `json-serialize', which
-see.  The returned object will be a vector, hashtable, or alist.  Its
-elements will be `:null', `:false', t, numbers, strings, or further
-vectors, hashtables, and alists.  If there are duplicate keys in an
-object, all but the last one are ignored.  If STRING doesn't contain a
-valid JSON object, an error of type `json-parse-error' is signaled.
-The keyword argument `:object-type' specifies which Lisp type is used
-to represent objects; it can be `hash-table' or `alist'.
-usage: (json-parse-string STRING &key (OBJECT-TYPE \\='hash-table))  */)
-  (ptrdiff_t nargs, Lisp_Object *args)
+see.  The returned object will be a vector, hashtable, alist, or
+plist.  Its elements will be `:null', `:false', t, numbers, strings,
+or further vectors, hashtables, alists, or plists.  If there are
+duplicate keys in an object, all but the last one are ignored.  If
+STRING doesn't contain a valid JSON object, an error of type
+`json-parse-error' is signaled.  The keyword argument `:object-type'
+specifies which Lisp type is used to represent objects; it can be
+`hash-table', `alist' or `plist'.
+usage: (json-parse-string STRING &key (OBJECT-TYPE \\='hash-table)) */)
+     (ptrdiff_t nargs, Lisp_Object *args)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
 
@@ -912,6 +959,7 @@ syms_of_json (void)
 
   DEFSYM (QCobject_type, ":object-type");
   DEFSYM (Qalist, "alist");
+  DEFSYM (Qplist, "plist");
 
   defsubr (&Sjson_serialize);
   defsubr (&Sjson_insert);
diff --git a/test/src/json-tests.el b/test/src/json-tests.el
index 09067bad8c..4a01dc57cc 100644
--- a/test/src/json-tests.el
+++ b/test/src/json-tests.el
@@ -69,7 +69,30 @@ 'json-tests--error
   (should-error (json-serialize '((1 . 2))) :type 'wrong-type-argument)
   (should-error (json-serialize '((a . 1) . b)) :type 'wrong-type-argument)
   (should-error (json-serialize '#1=((a . 1) . #1#)) :type 'circular-list)
-  (should-error (json-serialize '(#1=(a #1#)))))
+  (should-error (json-serialize '(#1=(a #1#))))
+
+  (should (equal (json-serialize '(:abc [1 2 t] :def :null))
+                 "{\"abc\":[1,2,true],\"def\":null}"))
+  (should (equal (json-serialize '(abc [1 2 t] :def :null))
+                 "{\"abc\":[1,2,true],\"def\":null}"))
+  (should-error (json-serialize '#1=(:a 1 . #1#)) :type 'circular-list)
+  (should-error (json-serialize '(:foo "bar" (unexpected-alist-key . 1)))
+                :type 'wrong-type-argument)
+  (should-error (json-serialize '((abc . "abc") :unexpected-plist-key "key"))
+                :type 'wrong-type-argument)
+  (should-error (json-serialize '(:foo bar :odd-numbered))
+                :type 'wrong-type-argument)
+  (should (equal
+           (json-serialize
+            (list :detect-hash-table #s(hash-table test equal data ("bla" "ble"))
+                  :detect-alist `((bla . "ble"))
+                  :detect-plist `(:bla "ble")))
+           "\
+{\
+\"detect-hash-table\":{\"bla\":\"ble\"},\
+\"detect-alist\":{\"bla\":\"ble\"},\
+\"detect-plist\":{\"bla\":\"ble\"}\
+}")))
 
 (ert-deftest json-serialize/object-with-duplicate-keys ()
   (skip-unless (fboundp 'json-serialize))
@@ -89,7 +112,9 @@ 'json-tests--error
       (should (equal (cl-sort (map-pairs actual) #'string< :key #'car)
                      '(("abc" . [9 :false]) ("def" . :null)))))
     (should (equal (json-parse-string input :object-type 'alist)
-                   '((abc . [9 :false]) (def . :null))))))
+                   '((abc . [9 :false]) (def . :null))))
+    (should (equal (json-parse-string input :object-type 'plist)
+                   '(:abc [9 :false] :def :null)))))
 
 (ert-deftest json-parse-string/string ()
   (skip-unless (fboundp 'json-parse-string))
-- 
2.17.0

Re: [PATCH] Accept plists when serializing and parsing JSON

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Sat, 02 Jun 2018 00:29:30 +0100
> 
> > Here I'd use intern_1 instead, it would allow you to avoid
> > unnecessarily consing Lisp objects.  (Yes, I realize that the same
> > comment applies to the existing code.)
> 
> Riight, intern_1 sounds good... I know I can't just malloc() stuff as
> usually right? But also, I have no idea what I'm doing, I aped this from
> somewhere where it looked more-or-less responsibly done.
> 
>   json_object_foreach (json, key_str, value)
>   {
>     USE_SAFE_ALLOCA;
>     char *keyword_key_str = SAFE_ALLOCA (1 + strlen(key_str) + 1);
>     keyword_key_str[0]=':';
>     strcpy(&keyword_key_str[1],key_str);
>     Lisp_Object key = intern_1(keyword_key_str, strlen(key_str)+1);
>     result = Fcons (key, result); /* build the plist as
>                                      value-key since
>                                      we're going to
>                                      reverse it in the
>                                      end.*/
>     result = Fcons (json_to_lisp (value, object_type), result);
>     SAFE_FREE ();
>   }

This looks OK to me, modulo a few stylistic gotchas:

  . use a temporary variable to compute strlen only once
  . leave one space _before_ an opening paren and around binary
    operators such as '+' and ',', as in, for example,

       intern_1 (keyword_key_str, strlen (key_str) + 1);
               ^                        ^         ^ ^
  . I personally prefer

        strcpy (keyword_key_str + 1, ...

     to

        strcpy (&keyword_key_str[1], ...

    although both do the same and are correct C
  . It is preferable to have long comments before the code, not at its
    side, split between several lines; and make the comment start with
    a capital letter, since it's a full sentence

Re: [PATCH] Accept plists when serializing and parsing JSON

[Philipp Stephani]

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

João Távora <joaotavora@gmail.com> schrieb am Sa., 2. Juni 2018 um
01:30 Uhr:

>
> New patch after my sig
>
>
Thanks, some more detailed comments:

test.texi:

When an alist or plist
+contains several elements with the same key, Emacs uses only the first
+element for serialization, in accordance with the behavior of
+@code{assq}.

"in accordance with ... assq and plist-get" (plist-get also uses the first
element)

+Note that @code{nil} is both a valid alist and a valid plist and
+represents the empty JSON object, @code{@{@}}, not @code{null},
+@code{false}, or an empty array, all of which are different JSON
+values.

This is a bit hard to parse, could you split it up into multiple sentences?

 or @code{plist} to use plists with keyword
+symbols as keys.

It's merely a somewhat common convention to use keywords (starting with :)
in plist keys. I think it's better to just use the same symbols, i.e.
neither add nor strip colons. That would also make the implementation
simpler.

You should also mention how the function distinguishes between alists and
plists.

+          /* If using plists, maybe strip the ":" from symbol-name */
+          if (is_plist &&
+              ':' == key_str[0] &&
+              key_str[1] ) key_str = &key_str[1];

As said above, I think it's better to remove this code (and maybe document
that colons aren't stripped). Especially the "maybe" part makes the
interface more complex and subtle. For example, a caller sees that (a 1 b
2) gets serialized to {"a": 1, "b": 2} and then might reasonably conclude
that (a 1 :a 2) gets serialized to {"a": 1, ":a": 2}, but that would be
wrong. It's more obvious and easier to understand to not treat colons
specially.

+Vectors will be converted to JSON arrays, and hashtables and alists to
+JSON objects.

Mention plists. (MIght be better to split this up into two sentences,
and/or switch to active voice.)

+                  char *keyword_key_str = SAFE_ALLOCA (1 + strlen(key_str)
+ 1);
+                  keyword_key_str[0]=':';

Again, no special treatment for colons please.

+see.  The returned object will be a vector, hashtable, alist, or
+plist.  Its elements will be `:null', `:false', t, numbers, strings,

Nit: you're using Oxford commas inconsistently. I'm fine either way, but
please stay consistent.

+  (should-error (json-serialize '#1=(:a 1 . #1#)) :type 'circular-list)

Add another unit test to check that circularity in values is detected (i.e.
'#1=(:a 1 :b . #1#)).

Otherwise this looks fine to me. I'm still not super convinced we need to
support plists at all for serialization, but the auto-detection looks
reliable and obvious enough.

[-- Attachment #2: Type: text/html, Size: 3795 bytes --]

Re: [PATCH] Accept plists when serializing and parsing JSON

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 2 Jun 2018 10:24:20 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> +see.  The returned object will be a vector, hashtable, alist, or
> +plist.  Its elements will be `:null', `:false', t, numbers, strings,
> 
> Nit: you're using Oxford commas inconsistently. I'm fine either way, but please stay consistent.

I see nothing inconsistent here, FWIW.  Maybe I, too, need to be
educated.

Re: [PATCH] Accept plists when serializing and parsing JSON

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 2. Juni 2018 um 11:00 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sat, 2 Jun 2018 10:24:20 +0200
> > Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> >
> > +see.  The returned object will be a vector, hashtable, alist, or
> > +plist.  Its elements will be `:null', `:false', t, numbers, strings,
> >
> > Nit: you're using Oxford commas inconsistently. I'm fine either way, but
> please stay consistent.
>
> I see nothing inconsistent here, FWIW.  Maybe I, too, need to be
> educated.
>

From the beginning of the patch:

+ Lisp hashtables, alists or plists

(i.e. no Oxford comma)

Then later:

+see.  The returned object will be a vector, hashtable, alist, or
+plist.  Its elements will be `:null', `:false', t, numbers, strings,

(i.e. Oxford comma)

There are some other inconsistencies.

This is obviously extremely minor.

[-- Attachment #2: Type: text/html, Size: 1630 bytes --]

Re: [PATCH] Accept plists when serializing and parsing JSON

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 2 Jun 2018 18:46:35 +0200
> Cc: joaotavora@gmail.com, emacs-devel@gnu.org
> 
> From the beginning of the patch:
> 
> + Lisp hashtables, alists or plists
> 
> (i.e. no Oxford comma)
> 
> Then later:
> 
> +see.  The returned object will be a vector, hashtable, alist, or
> +plist.  Its elements will be `:null', `:false', t, numbers, strings,
> 
> (i.e. Oxford comma)

I prefer the latter variant, but I don't think we are consistent in
that in our manuals.

Re: [PATCH] Accept plists when serializing and parsing JSON

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Sat, 02 Jun 2018 00:29:30 +0100
> 
> New patch after my sig

Thanks.  Apart of some missing spaces before opening paren, like here:

> +                  strcpy(&keyword_key_str[1],key_str);
> +                  Lisp_Object key = intern_1(keyword_key_str,strlen(key_str)+1);

This looks good to go, unless you still have unresolved issues wrt
comments by others.

Re: [PATCH] Accept plists when serializing and parsing JSON

[Philipp Stephani]

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

João Távora <joaotavora@gmail.com> schrieb am Di., 29. Mai 2018 um
16:59 Uhr:

> Hi,
>
> So I found this other thread
>
>   https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00667.html
>
> where plist support for json.c is briefly requested and discussed, but
> there didn't seem to be an overwhelming argument against it, so I had
> another look at the file and it didn't seem very hard or problematic to
> implement.
>

I'll try to respond to that thread as well, please give me some time.

Here's an older thread with a similar discussion:
https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00776.html
My primary argument was that there aren't good enough arguments for
supporting plists.


>
>
> PS: Take this opportunity to thank Eli and everyone else very much for
> the Emacs 26.1 release!
>
>
Indeed, thanks a lot Eli!

[-- Attachment #2: Type: text/html, Size: 1568 bytes --]