Add indentation offset guess for gyp emacs mode.

tools/emacs/gyp.el

Index: tools/emacs/gyp.el
diff --git a/tools/emacs/gyp.el b/tools/emacs/gyp.el
index 60619b52c83555344468dc30183f554fe6c71fc9..604d5a27184d31984b8d4dfd77cafe2e2f014559 100644
--- a/tools/emacs/gyp.el
+++ b/tools/emacs/gyp.el
@@ -23,7 +23,27 @@
                            (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/"
@@ -38,7 +58,8 @@
   "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)
 
@@ -223,14 +244,15 @@
                                 "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 "['\"]\\("