|
|
Created:
6 years, 3 months ago by Fabrice (no longer in Chrome) Modified:
6 years, 1 month ago CC:
gyp-developer_googlegroups.com Base URL:
https://chromium.googlesource.com/external/gyp.git@master Visibility:
Public. |
DescriptionAdd indentation offset guess for gyp emacs mode.
This fixes the indentation for some of the gyp files using 4 spaces
indentation instead of the usual 2.
Also adds "ldflags" in the list of keywords.
Patch Set 1 #
Total comments: 30
Patch Set 2 : Review comments #Messages
Total messages: 10 (1 generated)
fdegans@chromium.org changed reviewers: + gavinp@chromium.org, thakis@chromium.org
+gavinp for code review and comments. +thakis to actually land it after.
Thanks for making this better! Google does not have a public Emacs Lisp coding standard. We do have a published Common Lisp coding standard. I used it https://engdoc.corp.google.com/eng/doc/lispguide.xml , and where appropriate, I informed us from the GNU Emacs Lisp Reference Manual, at https://www.gnu.org/software/emacs/manual/html_mono/elisp.html . I think we should wrap our lines at 100 columns, per https://engdoc.corp.google.com/eng/doc/lispguide.xml?showone=Line_length&cl=h... , and I've commented on that basis. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el File tools/emacs/gyp.el (right): https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode26 tools/emacs/gyp.el:26: (- (first python-indent-levels) python-continuation-offset)))) Why this change? I say we just lose python-continuation-offset completely, and only use python-indent-offset. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode28 tools/emacs/gyp.el:28: (defun gyp-indent-guess-indent-offset () 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 ....) https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode30 tools/emacs/gyp.el:30: (interactive) Really? https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode35 tools/emacs/gyp.el:35: (let ((indentation)) 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)) https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode36 tools/emacs/gyp.el:36: ;; Find first line ending with an opening bracket Nit: I think "brace" is a better word to use here. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode37 tools/emacs/gyp.el:37: ;; that is not a comment. Nit: this comment fits on one line. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode38 tools/emacs/gyp.el:38: (when (re-search-forward "\\(^[[{]$\\|^.*[^#].*[[{]$\\)" nil nil) Nit: don't provide default values to &optional arguments. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode39 tools/emacs/gyp.el:39: (next-line 1) 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. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode40 tools/emacs/gyp.el:40: (setq indentation (current-indentation))) Nit: Avoid side effects. See https://engdoc.corp.google.com/eng/doc/lispguide.xml?showone=Mostly_Functiona... https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode41 tools/emacs/gyp.el:41: (if (and indentation (not (zerop indentation))) 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... https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode45 tools/emacs/gyp.el:45: (message "Can't guess gyp indent offset, using defaults: %s" Nit, grammar: defaults --> default, right? https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode46 tools/emacs/gyp.el:46: python-indent-offset)))))) Nit: This can go on the previous line. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode60 tools/emacs/gyp.el:60: python-indent-offset 2 I'm alarmed. Either you don't need to use make-local-variable above, or more likely, these are wrong... https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode61 tools/emacs/gyp.el:61: python-indent-guess-indent-offset nil) 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. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode247 tools/emacs/gyp.el:247: "include_dirs" "includes" "inputs" "ldflags" 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.
Thanks a lot Gavin! I modified the file with your comments, but kept the continuation variable because that is the one we want. There is probably a way to set all the local variables with one function call, which would be lisp-ier, but the syntax is alien to me. I had to call the python-indent-guess-indent-offset function in gyp-set-indentation because its invocation takes place before we set major-mode to gyp-mode. Essentially it's called twice, I haven't found a better way to do it. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el File tools/emacs/gyp.el (right): https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode26 tools/emacs/gyp.el:26: (- (first python-indent-levels) python-continuation-offset)))) On 2014/09/25 20:07:33, gavinp wrote: > Why this change? I say we just lose python-continuation-offset completely, and > only use python-indent-offset. 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. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode28 tools/emacs/gyp.el:28: (defun gyp-indent-guess-indent-offset () On 2014/09/25 20:07:33, gavinp wrote: > 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 ....) Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode30 tools/emacs/gyp.el:30: (interactive) On 2014/09/25 20:07:33, gavinp wrote: > Really? It's in python.el, this code is largely based around it. Do we really want to trust the FSF? https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode35 tools/emacs/gyp.el:35: (let ((indentation)) On 2014/09/25 20:07:32, gavinp wrote: > 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)) Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode36 tools/emacs/gyp.el:36: ;; Find first line ending with an opening bracket On 2014/09/25 20:07:33, gavinp wrote: > Nit: I think "brace" is a better word to use here. Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode37 tools/emacs/gyp.el:37: ;; that is not a comment. On 2014/09/25 20:07:33, gavinp wrote: > Nit: this comment fits on one line. Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode38 tools/emacs/gyp.el:38: (when (re-search-forward "\\(^[[{]$\\|^.*[^#].*[[{]$\\)" nil nil) On 2014/09/25 20:07:33, gavinp wrote: > Nit: don't provide default values to &optional arguments. Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode39 tools/emacs/gyp.el:39: (next-line 1) On 2014/09/25 20:07:33, gavinp wrote: > 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. Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode40 tools/emacs/gyp.el:40: (setq indentation (current-indentation))) On 2014/09/25 20:07:33, gavinp wrote: > Nit: Avoid side effects. See > https://engdoc.corp.google.com/eng/doc/lispguide.xml?showone=Mostly_Functiona... Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode41 tools/emacs/gyp.el:41: (if (and indentation (not (zerop indentation))) On 2014/09/25 20:07:33, gavinp wrote: > 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... Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode45 tools/emacs/gyp.el:45: (message "Can't guess gyp indent offset, using defaults: %s" On 2014/09/25 20:07:33, gavinp wrote: > Nit, grammar: defaults --> default, right? Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode46 tools/emacs/gyp.el:46: python-indent-offset)))))) On 2014/09/25 20:07:33, gavinp wrote: > Nit: This can go on the previous line. Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode60 tools/emacs/gyp.el:60: python-indent-offset 2 On 2014/09/25 20:07:32, gavinp wrote: > I'm alarmed. Either you don't need to use make-local-variable above, or more > likely, these are wrong... These are wrong. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode61 tools/emacs/gyp.el:61: python-indent-guess-indent-offset nil) On 2014/09/25 20:07:33, gavinp wrote: > 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. Done. https://codereview.chromium.org/598023002/diff/1/tools/emacs/gyp.el#newcode247 tools/emacs/gyp.el:247: "include_dirs" "includes" "inputs" "ldflags" On 2014/09/25 20:07:33, gavinp wrote: > 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. Yes just ldflags. My OCD compelled me to set everything to 80 columns. Done.
lgtm, thanks!
Thanks Gavin! Nico, can you land it?
Sure, can do, but shouldn't all gyp files just use 2 indent? Which ones don't?
Landed in gyp r1995
On 2014/10/28 16:34:54, Nico wrote: > Sure, can do, but shouldn't all gyp files just use 2 indent? Which ones don't? There's a bunch in blink. This one for instance: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
And thanks for landing it! Closing this. |