Add indentation offset guess for gyp emacs mode.

tools/emacs/gyp.el

 ;;; gyp.el - font-lock-mode support for gyp files.
 
 ;; Copyright (c) 2012 Google Inc. All rights reserved.
 ;; Use of this source code is governed by a BSD-style license that can be
 ;; found in the LICENSE file.
 
 ;; Put this somewhere in your load-path and
 ;; (require 'gyp)
 
 (require 'python)
 (require 'cl)
 
 (when (string-match "python-mode.el" (symbol-file 'python-mode 'defun))
   (error (concat "python-mode must be loaded from python.el (bundled with "
                  "recent emacsen), not from the older and less maintained "
                  "python-mode.el")))
 
 (defadvice python-indent-calculate-levels (after gyp-outdent-closing-parens
                                                  activate)
   "De-indent closing parens, braces, and brackets in gyp-mode."
   (when (and (eq major-mode 'gyp-mode)
              (string-match "^ *[])}][],)}]* *$"
                            (buffer-substring-no-properties
                             (line-beginning-position) (line-end-position))))
     (setf (first python-indent-levels)
-          (- (first python-indent-levels) python-indent-offset))))
+          (- (first python-indent-levels) python-continuation-offset))))

[gavinp, 2014/09/25 20:07:33]

Why this change? I say we just lose python-continuation-offset completely, and
only use python-indent-offset.

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Because I found out python-continutation-offset is the variable we actually
want.
See, python-indent-offset is the python indentation offset. But
continuation-offset is the offset set after a { or [ in Python, which is what
gyp indentation is based of.

+
+(defun gyp-indent-guess-indent-offset ()

[gavinp, 2014/09/25 20:07:33]

Maybe:

(defadvice python-indent-guess-indent-offset (around
gyp-indent-guess-indent-offset activate)
  "Guess correct indent offset in gyp-mode."
  (or (and (not (eq major-mode 'gyp-mode))
           ad-do-it))
      (save-excursion ....)

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

+  "Guess correct indent offset in gyp-mode."
+  (interactive)

[gavinp, 2014/09/25 20:07:33]

Really?

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

It's in python.el, this code is largely based around it. Do we really want to
trust the FSF?

+  (save-excursion
+    (save-restriction
+      (widen)
+      (goto-char (point-min))
+      (let ((indentation))

[gavinp, 2014/09/25 20:07:32]

I found this logic hard to follow. How about this instead?

;; Find first line ending with an opening brace that is not a comment.
(or (and (re-search-forward "\\(^[[{]$\\|^.*[^#].*[[{]$\\)")
         (forward-line)
         (/= (current-indentation) 0)
         (set (make-local-variable 'python-indent-offset)
(current-indentation)))
    (message "Can't guess gyp indent offset, using default: %s"
python-indent-offset))

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

+        ;; Find first line ending with an opening bracket

[gavinp, 2014/09/25 20:07:33]

Nit: I think "brace" is a better word to use here.

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

+        ;; that is not a comment.

[gavinp, 2014/09/25 20:07:33]

Nit: this comment fits on one line.

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

+        (when (re-search-forward "\\(^[[{]$\\|^.*[^#].*[[{]$\\)" nil nil)

[gavinp, 2014/09/25 20:07:33]

Nit: don't provide default values to &optional arguments.

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

+          (next-line 1)

[gavinp, 2014/09/25 20:07:33]

Nit: Don't use next-line
https://www.gnu.org/software/emacs/manual/html_mono/elisp.html#Programming-Tips
Nit: Don't provide default values to &optional arguments.

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

+          (setq indentation (current-indentation)))

[gavinp, 2014/09/25 20:07:33]

Nit: Avoid side effects. See
https://engdoc.corp.google.com/eng/doc/lispguide.xml?showone=Mostly_Functiona...

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

+        (if (and indentation (not (zerop indentation)))

[gavinp, 2014/09/25 20:07:33]

Nit: Don't use IF when one of the clauses would be a PROGN. See
https://engdoc.corp.google.com/eng/doc/lispguide.xml?showone=Conditional_Expr...

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

+            (progn
+              (set (make-local-variable 'python-indent-offset) indentation)
+              (set (make-local-variable 'python-continuation-offset) indentation))
+          (message "Can't guess gyp indent offset, using defaults: %s"

[gavinp, 2014/09/25 20:07:33]

Nit, grammar: defaults --> default, right?

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

+                   python-indent-offset))))))

[gavinp, 2014/09/25 20:07:33]

Nit: This can go on the previous line.

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

 
 (define-derived-mode gyp-mode python-mode "Gyp"
   "Major mode for editing .gyp files. See http://code.google.com/p/gyp/"
   ;; gyp-parse-history is a stack of (POSITION . PARSE-STATE) tuples,
   ;; with greater positions at the top of the stack. PARSE-STATE
   ;; is a list of section symbols (see gyp-section-name and gyp-parse-to)
   ;; with most nested section symbol at the front of the list.
   (set (make-local-variable 'gyp-parse-history) '((1 . (list))))
   (gyp-add-font-lock-keywords))
 
 (defun gyp-set-indentation ()
   "Hook function to configure python indentation to suit gyp mode."
   (setq python-continuation-offset 2
         python-indent-offset 2

[gavinp, 2014/09/25 20:07:32]

I'm alarmed. Either you don't need to use make-local-variable above, or more
likely, these are wrong...

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

These are wrong.

-        python-indent-guess-indent-offset nil))
+        python-indent-guess-indent-offset nil)

[gavinp, 2014/09/25 20:07:33]

Since our code basically duplicates python-indent-guess-indent-offset , why not
do it as advice instead of as a replacement here in the start up? Then we can
set this to t. See that suggestion in more detail up at line 28.

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Done.

+  (gyp-indent-guess-indent-offset))
 
 (add-hook 'gyp-mode-hook 'gyp-set-indentation)
 
 (add-to-list 'auto-mode-alist '("\\.gyp\\'" . gyp-mode))
 (add-to-list 'auto-mode-alist '("\\.gypi\\'" . gyp-mode))
 (add-to-list 'auto-mode-alist '("/\\.gclient\\'" . gyp-mode))
 
 ;;; Font-lock support
 
 (defconst gyp-dependencies-regexp
@@ skipping 164 matching lines @@
   (font-lock-add-keywords
    nil
    (list
     ;; Top-level keywords
     (list (concat "['\"]\\("
               (regexp-opt (list "action" "action_name" "actions" "cflags"
                                 "cflags_cc" "conditions" "configurations"
                                 "copies" "defines" "dependencies" "destination"
                                 "direct_dependent_settings"
                                 "export_dependent_settings" "extension" "files"
-                                "include_dirs" "includes" "inputs" "libraries"
-                                "link_settings" "mac_bundle" "message"
-                                "msvs_external_rule" "outputs" "product_name"
-                                "process_outputs_as_sources" "rules" "rule_name"
-                                "sources" "suppress_wildcard"
-                                "target_conditions" "target_defaults"
-                                "target_defines" "target_name" "toolsets"
-                                "targets" "type" "variables" "xcode_settings"))
+                                "include_dirs" "includes" "inputs" "ldflags"

[gavinp, 2014/09/25 20:07:33]

This is just adding "ldflags", no other changes, right?

This formatting makes seeing diffs hard. :(

As well, perhaps we should consider going to 100 columns here.

[Fabrice (no longer in Chrome), 2014/09/26 12:23:37]

Yes just ldflags. My OCD compelled me to set everything to 80 columns.
Done.

+                                "libraries" "link_settings" "mac_bundle"
+                                "message" "msvs_external_rule" "outputs"
+                                "product_name" "process_outputs_as_sources"
+                                "rules" "rule_name" "sources"
+                                "suppress_wildcard" "target_conditions"
+                                "target_defaults" "target_defines"
+                                "target_name" "toolsets" "targets" "type"
+                                "variables" "xcode_settings"))
               "[!/+=]?\\)") 1 'font-lock-keyword-face t)
     ;; Type of target
     (list (concat "['\"]\\("
               (regexp-opt (list "loadable_module" "static_library"
                                 "shared_library" "executable" "none"))
               "\\)") 1 'font-lock-type-face t)
     (list "\\(?:target\\|action\\)_name['\"]\\s-*:\\s-*['\"]\\([^ '\"]*\\)" 1
           'font-lock-function-name-face t)
     (list 'gyp-section-match
           (list 1 'font-lock-function-name-face t t) ; dependencies
           (list 2 'font-lock-variable-name-face t t) ; variables, conditions
           (list 3 'font-lock-constant-face t t) ; sources
           (list 4 'font-lock-preprocessor-face t t)) ; preprocessor
     ;; Variable expansion
     (list "<@?(\\([^\n )]+\\))" 1 'font-lock-variable-name-face t)
     ;; Command expansion
     (list "<!@?(\\([^\n )]+\\))" 1 'font-lock-variable-name-face t)
     )))
 
 (provide 'gyp)