Re: [elpa] master ba07fd7: New package: json-mode

Re: [elpa] master ba07fd7: New package: json-mode

[Stefan Monnier]

> +(defvar json-mode-syntax-table

Would it make sense to inherit some things (such as the syntax-table)
from js-mode?

> +    ;; Object names
> +    ("\\(\"[^\"]*\"\\)[[:blank:]]*:"
> +     (1 'json-mode-object-name-face))
> +    ;; Strings
> +    ("\"\\(\\\\.\\|[^\"]\\)*\""
> +     (0 font-lock-string-face))))

Are these meant to match strings that span several lines?
If no, the regexps are wrong (because they can match multiple lines).
If yes, this will fail in various circumstances
(see (info "(elisp)Font Lock Multiline") for more details).

> +(defun json-font-lock-syntactic-face-function (state)
> +  "Highlight comments only.
> +Strings are handled by `json-mode-font-lock-keywords', since we
> +want to highlight object name strings differently from ordinary
> +strings."
> +  (when (nth 4 state) font-lock-comment-face))

Why not do something like

     (cond
      ((nth 4 state) font-lock-comment-face)
      ((and (nth 3 state)
            (json-string-is-object-name-p (nth 8 state)))
       'json-mode-object-name-face)
      (t font-lock-string-face))

Where json-string-is-object-name-p could look like

    (defun json-string-is-object-name-p (startpos)
      (save-excursion
        (goto-char startpos)
        (and (eq (char-after) ?\")
             (condition-case nil
                 (progn (forward-sexp 1) t)
               (scan-error nil))
             (looking-at "[[:blank:]]*:"))))
                   

-- Stefan

Re: [elpa] master ba07fd7: New package: json-mode

[Simen Heggestøyl]

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

Hi Stefan, thanks for your comments.

On Sun, Dec 11, 2016 at 6:03 PM, Stefan Monnier 
<monnier@iro.umontreal.ca> wrote:
> Would it make sense to inherit some things (such as the syntax-table)
> from js-mode?

JSON's syntax is a subset of the JavaScript syntax, so I think it would
make sense. On the other hand, the JSON syntax is very simple, and
js-mode currently uses cc-mode to produce a syntax table which contains
much more than json-mode needs, so maybe it's OK to just define what we
need in json-mode. What do you think?

> Are these meant to match strings that span several lines?
> If no, the regexps are wrong (because they can match multiple lines).
> If yes, this will fail in various circumstances
> (see (info "(elisp)Font Lock Multiline") for more details).

No. They're removed in the proposed diff.

> Why not do something like [...]

Looks good to me. See the attached diff.

-- Simen

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP.patch --]
[-- Type: text/x-patch, Size: 1944 bytes --]

From 51dc7f55490a957bc4cb289c7f97b0688dd67dfe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Fri, 16 Dec 2016 17:05:53 +0100
Subject: [PATCH] WIP

---
 packages/json-mode/json-mode.el | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/packages/json-mode/json-mode.el b/packages/json-mode/json-mode.el
index defaae0..4c87a81 100644
--- a/packages/json-mode/json-mode.el
+++ b/packages/json-mode/json-mode.el
@@ -81,20 +81,28 @@
 (defvar json-mode-font-lock-keywords
   `(;; Constants
     (,(concat "\\<" (regexp-opt json-keywords) "\\>")
-     (0 font-lock-constant-face))
-    ;; Object names
-    ("\\(\"[^\"]*\"\\)[[:blank:]]*:"
-     (1 'json-mode-object-name-face))
-    ;; Strings
-    ("\"\\(\\\\.\\|[^\"]\\)*\""
-     (0 font-lock-string-face))))
+     (0 font-lock-constant-face))))
+
+(defun json-mode--string-is-object-name-p (startpos)
+  "Return t if STARTPOS is at the beginning of an object name."
+  (save-excursion
+    (goto-char startpos)
+    (and (eq (char-after) ?\")
+         (condition-case nil
+             (progn (forward-sexp 1) t)
+           (scan-error nil))
+         (looking-at "[[:blank:]]*:"))))
 
 (defun json-font-lock-syntactic-face-function (state)
-  "Highlight comments only.
-Strings are handled by `json-mode-font-lock-keywords', since we
-want to highlight object name strings differently from ordinary
-strings."
-  (when (nth 4 state) font-lock-comment-face))
+  "Determine which face to use for strings and comments.
+Object names receive the face `json-mode-object-name-face' to
+distinguish them from other strings."
+  (cond
+   ((nth 4 state) font-lock-comment-face)
+   ((and (nth 3 state)
+         (json-mode--string-is-object-name-p (nth 8 state)))
+    'json-mode-object-name-face)
+   (t font-lock-string-face)))
 
 (defconst json-mode--smie-grammar
   (smie-prec2->grammar
-- 
2.10.2

Re: [elpa] master ba07fd7: New package: json-mode

[Stefan Monnier]

>> Would it make sense to inherit some things (such as the syntax-table)
>> from js-mode?
> JSON's syntax is a subset of the JavaScript syntax, so I think it would
> make sense.  On the other hand, the JSON syntax is very simple, and
> js-mode currently uses cc-mode to produce a syntax table which contains
> much more than json-mode needs, so maybe it's OK to just define what we
> need in json-mode.  What do you think?

I tend to prefer reusing another package's code, even if that other
package is "overkill" unless there's a reason to be worried about the
resource cost.

> Looks good to me. See the attached diff.

Sounds great, thanks,


        Stefan