From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Philipp Stephani Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Accept plists when serializing and parsing JSON Date: Sat, 2 Jun 2018 10:24:20 +0200 Message-ID: References: <87sh6awls5.fsf@gmail.com> <83o9gug811.fsf@gnu.org> <87d0xaozl1.fsf@gmail.com> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="000000000000124c07056da46e62" X-Trace: blaine.gmane.org 1527927761 16346 195.159.176.226 (2 Jun 2018 08:22:41 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 2 Jun 2018 08:22:41 +0000 (UTC) Cc: Eli Zaretskii , emacs-devel@gnu.org To: =?UTF-8?B?Sm/Do28gVMOhdm9yYQ==?= Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Jun 02 10:22:37 2018 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1fP1o2-00043l-Lo for ged-emacs-devel@m.gmane.org; Sat, 02 Jun 2018 10:22:34 +0200 Original-Received: from localhost ([::1]:58704 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fP1q8-0004if-0C for ged-emacs-devel@m.gmane.org; Sat, 02 Jun 2018 04:24:44 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:34968) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fP1q0-0004iW-Lq for emacs-devel@gnu.org; Sat, 02 Jun 2018 04:24:38 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fP1pz-0007O9-6y for emacs-devel@gnu.org; Sat, 02 Jun 2018 04:24:36 -0400 Original-Received: from mail-ot0-x242.google.com ([2607:f8b0:4003:c0f::242]:45720) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fP1px-0007Iz-4N; Sat, 02 Jun 2018 04:24:33 -0400 Original-Received: by mail-ot0-x242.google.com with SMTP id a5-v6so816776otf.12; Sat, 02 Jun 2018 01:24:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=VX4xFjSWE2J3wmQza7x6VxgjtEyBOVtY5D7zeg1mc+w=; b=NNXZXiBY58DL4eFNjktB+3ZvWCsxDgXcnKn38TWOYkH9t5LwCwpzkL+/+Q4OMktT93 ifv3hOQJwwBedP8AT0V0hsTS38NF3CoDR0hO0mFJ+BvKMGUrO7Vz+mhvace2W2GibGMm LZs13qY78S2Ru+bt3GMor8gTvRhN1v4v+50KcYVklaExSVSeKrFEIfr4b+V6EIOKClSn 3HR9axq6nDy7Yps+QyX+GlkrqGuh+slDGklFU0Ihe/+3nt/6MZnikAkTEoxdP+u9i5SV Jt0xTPgUVkjvTrxd/FYojThKcnHn65udbYju3gWyiphwN5vYIw8oiG681ilpJ+JFYQO+ 5YZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VX4xFjSWE2J3wmQza7x6VxgjtEyBOVtY5D7zeg1mc+w=; b=jCZ9tY+Lf4YkSBQ3dpS05bGjDYY6518x1I8aHdf9txAZrCCA9jQSsJNiUyzvJB25Cs igbk5urG6nhBX9IyhXpng3pBWPjys5Bx7YHMXHoR7dpSqfTdPQDu5BE0G4qwOg0BroFg JZvN/HCjJXtRwtuzder06USklT4zlDVilb8pvoua7prCdk7qBhvX5h18XNid4poBmVAp YzQ2WrU2e0waHp47QsztBom6WVmM3rO2Jdtrn5eYeXmTB6cwy7KqE7XJqGdK0guIEqC+ 1d22UcD5hMAgnKqhmbjZF01r6CsvthK7FyCwNtW4ldIqqdz4C+3RXqlFbUOfoyTkSXj2 PkUQ== X-Gm-Message-State: APt69E2aPhZk0HqOJD6wAnNT20a7swFgIJ76Z/P4pig1wOi44SG4GZAe rEUlZ2VzHT9yH1i+BqPyzaSGvVUGkmy6vPFdxqE= X-Google-Smtp-Source: ADUXVKKclH47EfywBi+ik8vNrIKnJ5ypnng2eAJzu3Z17NoT68X3I7C3pr8VIXUPjZBEAC1fV9OHjOIIx1LT6InWnnU= X-Received: by 2002:a9d:282e:: with SMTP id m43-v6mr9467191otb.393.1527927872308; Sat, 02 Jun 2018 01:24:32 -0700 (PDT) In-Reply-To: <87d0xaozl1.fsf@gmail.com> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4003:c0f::242 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:225915 Archived-At: --000000000000124c07056da46e62 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Jo=C3=A3o T=C3=A1vora schrieb am Sa., 2. Juni 2018 u= m 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 && + ':' =3D=3D key_str[0] && + key_str[1] ) key_str =3D &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 =3D SAFE_ALLOCA (1 + strlen(key_st= r) + 1); + keyword_key_str[0]=3D':'; 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=3D(:a 1 . #1#)) :type 'circular-list) Add another unit test to check that circularity in values is detected (i.e. '#1=3D(: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. --000000000000124c07056da46e62 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


Jo=C3= =A3o T=C3=A1vora <joaotavora@gma= il.com> schrieb am Sa., 2. Juni 2018 um 01:30=C2=A0Uhr:

New patch after my sig


Thanks, some more detailed comments:

test.texi:

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

"i= n accordance with ... assq and plist-get" (plist-get also uses the fir= st element)

+Note that @code{nil} is both a v= alid alist and a valid plist and
+represents the empty JSON objec= t, @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 s= entences?

=C2=A0or @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 simp= ler.

You should also mention how the function dist= inguishes between alists and plists.

+=C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* If using plists, maybe strip the ":&quo= t; from symbol-name */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (is= _plist &&
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 ':' =3D=3D key_str[0] &&
+=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 key_str[1] ) key_str =3D &key_str[1];

As said above, I think it's better to rem= ove this code (and maybe document that colons aren't stripped). Especia= lly 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 arra= ys, and hashtables and alists to
+JSON objects.=C2=A0=C2=A0
=

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

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 char *key= word_key_str =3D SAFE_ALLOCA (1 + strlen(key_str) + 1);
+=C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 keyword_key_str[0]= =3D':';

Again, no special treatment = for colons please.

+see.=C2=A0 The returned o= bject will be a vector, hashtable, alist, or
+plist.=C2=A0 Its el= ements will be `:null', `:false', t, numbers, strings,
<= div>
Nit: you're using Oxford commas inconsistently. I= 9;m fine either way, but please stay consistent.

<= div>+=C2=A0 (should-error (json-serialize '#1=3D(:a 1 . #1#)) :type = 9;circular-list)

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

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