Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10264008:
Support for gyp-mode in Emacs. (Closed)
|
Created:
8 years, 7 months ago by John Knottenbelt Modified:
8 years, 3 months ago CC:
gyp-developer_googlegroups.com Base URL:
http://gyp.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionEmacs mode for gyp: font-lock edition.
Patch Set 1 #
Total comments: 65
Patch Set 2 : #Patch Set 3 : Emacs mode for gyp #
Total comments: 52
Patch Set 4 : '' #
Total comments: 1
Patch Set 5 : Fix nit. Remove auto-mode-alist test. #Patch Set 6 : '' #Patch Set 7 : Fix license on run-unit-tests.sh (presubmit) #
Messages
Total messages: 16 (0 generated)
Thanks for doing this! https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el File tools/emacs/gyp.el (right): https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:5: ;;; Thanks to Ami Fischman for python derivation and indentation code. You're welcome, but this is unnecessary. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:10: ;;; NB: You need to customize variable which-func-modes to add in gyp-mode in order 80-col, here and below. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:22: "De-indent closing parens, braces, and brackets in python and derived modes." This affects all python-related modes. Personally, I think this is a good thing, but I'm not sure a gyp-mode should be making that change (esp. in a world where e.g. google3 build-file-mode defines a similar advice, and you end up with a minus-4 instead of a minus-2). Since there's no such thing as buffer-local advice, I think the best we can do here is wrap add an (and (eq major-mode 'gyp-mode) to the if test (and fix the docstring to reflect this). https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:26: (setq ad-return-value (- ad-return-value 2)))) If you're interested, you could try upstreaming the goal (if not the impl) to emacs' python.el. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:29: "Major mode for editing .gyp files." Might have docstring point to http://code.google.com/p/gyp/ https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:36: ;;; Font-lock support I eval'd this file in an emacs -Q (23.1.1, default on glucid), loaded chromium's media/media.gyp, customized which-func-modes, and enabled which-func-mode, and observed various opportunities for improvement: 1. sources are highlighted inconsistently 2. sources highlighting changes when I mouse-click on one of them 3. which-func-mode claims point is in the previous target when not in any target. Screenshots (in this order) are at http://www/~fischman/no_crawl/gyp-mode/ (internal-only, for others on the reviewlog). https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:39: (make-local-variable 'gyp-parse-point) m-l-v returns its argument, so you can set & m-l-v in a single expression: (set (make-local-variable 'gyp-parse-point) 1) (this usage has the advantage of making sure that everything you end up setting has indeed been m-l-v'd, and not been subject to typos) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:44: gyp-parse-history '((1 . (top))))) can you either set g-p-h in terms of g-p-p & g-p-s, or else set each of the latter two in terms of g-p-h? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:46: (defun gyp-section-name (section) Please provide docstrings (and document args) for all your defuns. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:55: (defun gyp-find-earlier-parse-state (target-point) Is this idiomatic font-lock-related parsing? I'm finding it somewhat hard to follow. Might be useful to add comments explaining the data-structures you're using and their life-cycle vis-a-vis the various functions below (or some other layout explaining how this supports font-lock). https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:66: (defun gyp-parse-to (target-point stop-after-string) This is some serious business to not have any testing for. At least can we have a golden-file-based test that checks in a moderately-complex .gyp file and asserts expectations about its parse state? (if you're looking for a candidate, I recommend chromium's media/media.gyp :)) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:67: (cond ((and gyp-parse-point (< target-point gyp-parse-point)) This confused me greatly. Would this be equivalent but much clearer as: (unless (and gyp-parse-point (< target-point gyp-parse-point) (gyp-find-earlier-parse-state target-point)) ? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:67: (cond ((and gyp-parse-point (< target-point gyp-parse-point)) If gyp-parse-point is nil then the parse-partial-sexp below will blow up, no? (IOW what are you guarding against here?) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:73: (let ((state (parse-partial-sexp gyp-parse-point I *think* the goal of this is: parse up to the first of: opener or closer of any of [](){}""'' IWBN to have a comment to that effect (or to the real effect, if I'm misreading the code). https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:77: nil))) optional, defaults to nil, so why specify? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:81: ;; (message "Examining %s" ch) Either promote this sort of message to a debugging facility (message only when a debugging var is set) or drop the commented-out versions. (personally I enjoy flymake-log-level / flymake-log 's approach to this problem) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:82: (cond ((looking-at-p "['\"]") This is the "else" clause, right? If so shouldn't it be outdented by 2 spaces to emph that fact? (lisp style I'm used to is: (if test then-form else-form-1 else-form-2 ...) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:87: ;; and the limit. Consequently we need to introduce a 'in-string' I think this paragraph wants to be rewritten to just explain that a new parse (using forward-sexp) is necessary. I'm not sure what the TODO is here. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:93: (if stop-after-string The only call-site to this function passes t for this param, so why have a param? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:95: ((looking-at-p "[{\[]") Backslash is unnecessary. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:107: (set-match-data Ummm, at *least* a comment 'splaining what you're doing, please. Is there some reason to hack the emacs internal match-data (presumably to fake the string-match's from gyp-section-name?) instead of storing your data in your own variable and setting that variable following the other related string-match's you want to be using on par with this data? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:114: ;; other sources wat? :) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:117: (if (eq (car gyp-parse-sections) 'other) I'm surprised this form doesn't have an else-clause. Is it intentional that g-s-a-p return nil? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:122: (defun gyp-set-match-data (n) Sneaky! Definitely at least a comment warranted, but preferably avoid the need for this with comment at l.107 above. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:148: ;;; so that we don't have multiple font-lock modes competing with each other. Comment out of date? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:154: (list (concat "['\"]\\(" Replace \\( (and \\) below) with PAREN arg to regexp-opt? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:156: "action" Hopefully these come from someplace canonical (http://code.google.com/p/gyp/wiki/GypLanguageSpecification ?). Can you point to that from here? (to help future updaters of this file keep it in sync w/ an evolving gyp). https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:157: "action_name" personal preference: I'd save vertical space by space-separating this list (like borg-mode.el l.51 does). https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:195: "[!/+=]?\\)") 1 'font-lock-keyword-face t) Shouldn't \\) be followed by ['\"] for the closing quote to match the opener above? (actually my testing says it's not necessary, since the opening quote is not colorized like the section name, but I'm not sure why) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:204: "\\)") 1 'font-lock-type-face t) ditto missing closing quote https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:205: (list "\\(?:target\\|action\\)_name['\"]\\s-*:\\s-*['\"]\\([^ '\"]*\\)" 1 'font-lock-function-name-face t) any reason not to include the opening ['\"] preceding target_name and/or the trailing ['\"] following the name of the action/target? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:205: (list "\\(?:target\\|action\\)_name['\"]\\s-*:\\s-*['\"]\\([^ '\"]*\\)" 1 'font-lock-function-name-face t) curious why you chose to do this for action_name but not promote action_name to which-func-mode. If you did the latter then hopefully you could reuse gyp-target-name-regexp here. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:208: (list 2 'font-lock-variable-name-face t t) ; variables, conditions I don't understand the 2/3/4 params here and below. None of the regexps in gyp-section-match have more than one match group. Also I don't understand how this works at all, given that string-match-p doesn't modify match-data. (???) (in fact, adding an early ((t nil)) to the cond in gyp-section-match, I can't detect a behavior difference?) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:211: (list "<@?(\\([^\n )]+\\))" 1 'font-lock-variable-name-face t) ; Variable expansion Here and below, any reason not to include the open/close quoets? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:217: ;;; TODO(jknotten): what? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:221: (unless (search-forward-regexp "[{}\[\]]" nil t) Backslashes are wrong here ([ doesn't need it, ] would need two if it wasn't the last element of the char class, making it need none). https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:221: (unless (search-forward-regexp "[{}\[\]]" nil t) Comment this is trying to take care of incomplete gyp files? (or else what it's doing) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:224: (cond ((search-backward-regexp gyp-target-name-regexp nil t) This seems crazy to me, but maybe I haven't thought about it enough. My naive approach would be to say: 0. back up out of a string if inside one; then: 1. Use up-list's machinery to figure out the current level of parens 2. search within those bounds for gyp-target-name-regexp and use it if found. 3. else (backward-up-list) and go to 1. That would fix the spurious point at targets that point follows but is not in the parens of (case #3 in the screenshots list below). https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:237: (defun gyp-end-of-defun () Better as gyp-beginning-of-defun followed by forward-sexp ? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:260: (defconst gyp-target-name-regexp "'target_name['\"]\\s-*:\\s-*['\"]\\([^ '\"]*\\)") belongs before first use. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:268: (cons (cons (match-string-no-properties 1) (car (match-data 1))) index))) s/car+match-data/match-beginning/ ? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:276: (defun gyp-initialise () Any reason not to in-line this (and gyp-make-local-vars) into the define-derived-mode? (this would also allow me to avoid having to s/s/z/ ;))
Thanks for review, Ami. This update is a work in progress and there are still quite a few items from the original review still to be done (in later update). https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el File tools/emacs/gyp.el (right): https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:5: ;;; Thanks to Ami Fischman for python derivation and indentation code. On 2012/04/30 17:34:22, Ami Fischman wrote: > You're welcome, but this is unnecessary. Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:10: ;;; NB: You need to customize variable which-func-modes to add in gyp-mode in order On 2012/04/30 17:34:22, Ami Fischman wrote: > 80-col, here and below. Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:22: "De-indent closing parens, braces, and brackets in python and derived modes." On 2012/04/30 17:34:22, Ami Fischman wrote: > This affects all python-related modes. > Personally, I think this is a good thing, but I'm not sure a gyp-mode should be > making that change (esp. in a world where e.g. google3 build-file-mode defines a > similar advice, and you end up with a minus-4 instead of a minus-2). > > Since there's no such thing as buffer-local advice, I think the best we can do > here is wrap add an > (and (eq major-mode 'gyp-mode) > to the if test (and fix the docstring to reflect this). Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:29: "Major mode for editing .gyp files." On 2012/04/30 17:34:22, Ami Fischman wrote: > Might have docstring point to http://code.google.com/p/gyp/ Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:36: ;;; Font-lock support I'm missing a (require 'cl) to provide some needed functions such as caddr. I didn't get this problem because I had 'cl required by my .emacs. On 2012/04/30 17:34:22, Ami Fischman wrote: > I eval'd this file in an emacs -Q (23.1.1, default on glucid), loaded chromium's > media/media.gyp, customized which-func-modes, and enabled which-func-mode, and > observed various opportunities for improvement: > 1. sources are highlighted inconsistently > 2. sources highlighting changes when I mouse-click on one of them > 3. which-func-mode claims point is in the previous target when not in any > target. > > Screenshots (in this order) are at http://www/%7Efischman/no_crawl/gyp-mode/ > (internal-only, for others on the reviewlog). https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:39: (make-local-variable 'gyp-parse-point) On 2012/04/30 17:34:22, Ami Fischman wrote: > m-l-v returns its argument, so you can set & m-l-v in a single expression: > (set (make-local-variable 'gyp-parse-point) 1) > > (this usage has the advantage of making sure that everything you end up setting > has indeed been m-l-v'd, and not been subject to typos) Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:39: (make-local-variable 'gyp-parse-point) On 2012/04/30 17:34:22, Ami Fischman wrote: > m-l-v returns its argument, so you can set & m-l-v in a single expression: > (set (make-local-variable 'gyp-parse-point) 1) > > (this usage has the advantage of making sure that everything you end up setting > has indeed been m-l-v'd, and not been subject to typos) Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:44: gyp-parse-history '((1 . (top))))) Done. Thanks for suggesting this! On 2012/04/30 17:34:22, Ami Fischman wrote: > can you either set g-p-h in terms of g-p-p & g-p-s, or else set each of the > latter two in terms of g-p-h? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:67: (cond ((and gyp-parse-point (< target-point gyp-parse-point)) We can remove this altogether as gyp-find-earlier-parse-state is called before gyp-parse-to in gyp-section-match. On 2012/04/30 17:34:22, Ami Fischman wrote: > This confused me greatly. > Would this be equivalent but much clearer as: > (unless (and gyp-parse-point (< target-point gyp-parse-point) > (gyp-find-earlier-parse-state target-point)) > ? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:77: nil))) On 2012/04/30 17:34:22, Ami Fischman wrote: > optional, defaults to nil, so why specify? Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:81: ;; (message "Examining %s" ch) I've replaced these with comments where appropriate and removed them otherwise. I also like flymake's approach, but I wonder if there would be any function call overhead to the logging function, which I would like to avoid in the parsing function. Perhaps a macro-based approach would avoid that altogether? On 2012/04/30 17:34:22, Ami Fischman wrote: > Either promote this sort of message to a debugging facility (message only when a > debugging var is set) or drop the commented-out versions. > (personally I enjoy flymake-log-level / flymake-log 's approach to this problem) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:82: (cond ((looking-at-p "['\"]") On 2012/04/30 17:34:22, Ami Fischman wrote: > This is the "else" clause, right? If so shouldn't it be outdented by 2 spaces > to emph that fact? (lisp style I'm used to is: > (if test > then-form > else-form-1 > else-form-2 > ...) Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:93: (if stop-after-string You're right. gyp-section-match is only supposed to be returning highlighting between (point) and limit, but if we haven't parsed up to (point) it will return matches before then. This is what the stop-after-string was supposed to help with -- (gyp-parse-to (point)) and then continue on and parse stopping on strings. I think that we can remove this parameter and put the logic in gyp-section-match if it is actually needed. On 2012/04/30 17:34:22, Ami Fischman wrote: > The only call-site to this function passes t for this param, so why have a > param? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:117: (if (eq (car gyp-parse-sections) 'other) You could be in a section that we don't care about - perhaps the top level, target context etc. Will make this clearer with doc string. On 2012/04/30 17:34:22, Ami Fischman wrote: > I'm surprised this form doesn't have an else-clause. Is it intentional that > g-s-a-p return nil? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:148: ;;; so that we don't have multiple font-lock modes competing with each other. What'd I like to do at some point in the future is to remove the font-lock-add-keywords below and build it in to a more context-aware function -- perhaps gyp-section-match above, so that the font-lock system only needs to scan through the text once (it seems that it scans through the text once for each font-lock entry). On 2012/04/30 17:34:22, Ami Fischman wrote: > Comment out of date? https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:195: "[!/+=]?\\)") 1 'font-lock-keyword-face t) I'm not colouring the quotes (I set the match data specially to avoid it) - but if it's stylistically preferred we can do this. On 2012/04/30 17:34:22, Ami Fischman wrote: > Shouldn't \\) be followed by ['\"] for the closing quote to match the opener > above? > (actually my testing says it's not necessary, since the opening quote is not > colorized like the section name, but I'm not sure why) > https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:260: (defconst gyp-target-name-regexp "'target_name['\"]\\s-*:\\s-*['\"]\\([^ '\"]*\\)") On 2012/04/30 17:34:22, Ami Fischman wrote: > belongs before first use. Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:260: (defconst gyp-target-name-regexp "'target_name['\"]\\s-*:\\s-*['\"]\\([^ '\"]*\\)") On 2012/04/30 17:34:22, Ami Fischman wrote: > belongs before first use. Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:268: (cons (cons (match-string-no-properties 1) (car (match-data 1))) index))) On 2012/04/30 17:34:22, Ami Fischman wrote: > s/car+match-data/match-beginning/ ? Done. https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:276: (defun gyp-initialise () Good suggestion! On 2012/04/30 17:34:22, Ami Fischman wrote: > Any reason not to in-line this (and gyp-make-local-vars) into the > define-derived-mode? > > (this would also allow me to avoid having to s/s/z/ ;)) https://chromiumcodereview.appspot.com/10264008/diff/1/tools/emacs/gyp.el#new... tools/emacs/gyp.el:276: (defun gyp-initialise () On 2012/04/30 17:34:22, Ami Fischman wrote: > Any reason not to in-line this (and gyp-make-local-vars) into the > define-derived-mode? > > (this would also allow me to avoid having to s/s/z/ ;)) Done.
In the interest of trying to move a bit more quickly on this, I removed the which-mode support - it can be added later. A golden sample test test has been added which should detect any unintentional changes to the fontification algorithm. Thanks for your patience! http://codereview.chromium.org/10264008/diff/1/tools/emacs/gyp.el File tools/emacs/gyp.el (right): http://codereview.chromium.org/10264008/diff/1/tools/emacs/gyp.el#newcode122 tools/emacs/gyp.el:122: (defun gyp-set-match-data (n) On 2012/04/30 17:34:22, Ami Fischman wrote: > Sneaky! Definitely at least a comment warranted, but preferably avoid the need > for this with comment at l.107 above. Done. http://codereview.chromium.org/10264008/diff/1/tools/emacs/gyp.el#newcode217 tools/emacs/gyp.el:217: ;;; TODO(jknotten): Removed. On 2012/04/30 17:34:22, Ami Fischman wrote: > what?
http://codereview.chromium.org/10264008/diff/8001/tools/emacs/README File tools/emacs/README (right): http://codereview.chromium.org/10264008/diff/8001/tools/emacs/README#newcode5 tools/emacs/README:5: $ git clone git://github.com/jknotten/gyp-mode.git ?? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/README#newcode9 tools/emacs/README:9: (setq load-path (cons "~/.emacs.d/gyp-mode" load-path)) I assume you're planning to change this to point to .../chromium/src/tools/emacs instead? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/README#newcode12 tools/emacs/README:12: Restart emacs and you should be all set. eval-region'ing is enough, but I don't know if you want to get into that. http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el File tools/emacs/gyp-tests.el (right): http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:6: (setq load-path (cons "." load-path)) probably a bad idea considering this will be persistent for the emacs session (and CWD will be changing) for people eval'ing this test in a persistent session (not through the run shellscript) http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:8: ;;(load-library "subr") drop these two lines? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:9: (require 'gyp) load-library would allow you to avoid messing w/ the load-path. http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:11: ;;; When running with ert, for some reason these functions don't get ?? Just (require 'subr) at the top doesn't get you this? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:14: "\ ?? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:25: (defun fontify (filename) drop? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:40: (read (current-buffer)))) You could replace this read with simply asserting that (fontify sample) string='s the golden .fontified string. http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:44: (find-file "media.gyp") with-temp-buffer http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:44: (find-file "media.gyp") I worry about what if media.gyp is already opened in another buffer. Any reason not to use /tmp/<random>.gyp here? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:63: ;; Note that this doesn't compare the text properties which Comment out of date? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:69: (text-face-properties fontified))) I think my suggestion above replaces both of these "should"s. http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:79: (write-file (concat filename ".fontified"))))) (with-temp-file (concat filename ".fontified") (insert (fontify filename))) http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:83: (dolist (sample samples) (create-golden-sample "media.gyp"))) s/"media.gyp"/sample/ http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp-tests.el#ne... tools/emacs/gyp-tests.el:87: ;;(message "Golden sample: %s" (fontify "/Users/jknotten/bin/splitPDF.py")) drop these two http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp.el File tools/emacs/gyp.el (right): http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp.el#newcode54 tools/emacs/gyp.el:54: (cond ((string-match-p "^\\(dependencies\\|export_dependent_settings\\)" section) 'dependencies) You might enjoy regexp-opt http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp.el#newcode55 tools/emacs/gyp.el:55: ((string-match-p "^\\(action\\|sources[!/]?\\|[a-zA-Z0-9]*_sources[!/]?\\|files\\|include_dirs\\|includes\\|libraries\\|inputs\\|outputs\\)$" section) 'sources) 80-cols (here and elsewhere)? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp.el#newcode79 tools/emacs/gyp.el:79: (setq gyp-parse-history (cdr gyp-parse-history))) replace w/ call to gyp-invalidate-parse-states-after ? (possibly also s/>/>=/ in that function?) http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp.el#newcode84 tools/emacs/gyp.el:84: to gyp-parse-state-history. Stops after each string literal has been "Stops" and "up to" appear in conflict here. http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp.el#newcode134 tools/emacs/gyp.el:134: ;; other none sources What are these about? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp.el#newcode143 tools/emacs/gyp.el:143: including the "", and the first group to be the contents of the string. interesting use of "" inside a "-quoted string :) http://codereview.chromium.org/10264008/diff/8001/tools/emacs/gyp.el#newcode147 tools/emacs/gyp.el:147: (set-match-data I'm still distressed by your clobbering of the (non-gyp-specific) match-data. Is it necessary? http://codereview.chromium.org/10264008/diff/8001/tools/emacs/media.gyp File tools/emacs/media.gyp (right): http://codereview.chromium.org/10264008/diff/8001/tools/emacs/media.gyp#newcode1 tools/emacs/media.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. Move this file and its .fontified sibling into a testdata/ dir?
oh, also CL description in rietveld needs to drop mention of which-*
Pardon the drive-by. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el File tools/emacs/gyp.el (right): https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el#... tools/emacs/gyp.el:27: (setq python-continuation-offset 2 Drive-by: this is vulnerable to a parent mode hook (say, python-mode-hook) overwriting these values. A fix is to pull setting these variables out into a function added to gyp-mode-hook, which will get run after any parent mode hooks.
https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/README File tools/emacs/README (right): https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/README#... tools/emacs/README:5: $ git clone git://github.com/jknotten/gyp-mode.git Removed. On 2012/07/19 22:20:08, Ami Fischman wrote: > ?? https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/README#... tools/emacs/README:9: (setq load-path (cons "~/.emacs.d/gyp-mode" load-path)) On 2012/07/19 22:20:08, Ami Fischman wrote: > I assume you're planning to change this to point to .../chromium/src/tools/emacs > instead? Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/README#... tools/emacs/README:12: Restart emacs and you should be all set. On 2012/07/19 22:20:08, Ami Fischman wrote: > eval-region'ing is enough, but I don't know if you want to get into that. Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... File tools/emacs/gyp-tests.el (right): https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:6: (setq load-path (cons "." load-path)) On 2012/07/19 22:20:08, Ami Fischman wrote: > probably a bad idea considering this will be persistent for the emacs session > (and CWD will be changing) for people eval'ing this test in a persistent session > (not through the run shellscript) Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:8: ;;(load-library "subr") On 2012/07/19 22:20:08, Ami Fischman wrote: > drop these two lines? Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:9: (require 'gyp) On 2012/07/19 22:20:08, Ami Fischman wrote: > load-library would allow you to avoid messing w/ the load-path. I fixed this up in the shell script. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:11: ;;; When running with ert, for some reason these functions don't get It turned out that I was having difficulty here because I was running the tests on emacs 22 on my mac. Switched to running with 24.1 and this is no longer necessary, so removed. On 2012/07/19 22:20:08, Ami Fischman wrote: > ?? Just > (require 'subr) > at the top doesn't get you this? https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:14: "\ Removed. On 2012/07/19 22:20:08, Ami Fischman wrote: > ?? https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:25: (defun fontify (filename) On 2012/07/19 22:20:08, Ami Fischman wrote: > drop? Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:40: (read (current-buffer)))) Unfortunately string= doesn't compare string properties, and the result of (fontify sample) is a string with associated properties. One possibility is to use equal-including-properties, but I want to take advantage of ert's explanation functions which describe exactly where the mismatch is. There is ert-equal-including-properties, but it is very slow, which is why I have gone with the list of properties comparison approach (much faster and gives you the position at which there was a difference). On 2012/07/19 22:20:08, Ami Fischman wrote: > You could replace this read with simply asserting that (fontify sample) > string='s the golden .fontified string. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:44: (find-file "media.gyp") I wonder if perhaps we shouldn't just remove this test? It tests auto-mode-list functionality which I think is already pretty solid. What do you think? On 2012/07/19 22:20:08, Ami Fischman wrote: > with-temp-buffer https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:63: ;; Note that this doesn't compare the text properties which On 2012/07/19 22:20:08, Ami Fischman wrote: > Comment out of date? Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:63: ;; Note that this doesn't compare the text properties which On 2012/07/19 22:20:08, Ami Fischman wrote: > Comment out of date? Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:79: (write-file (concat filename ".fontified"))))) Done. I need to use (print) so that the properties are output into the .fontfied file. On 2012/07/19 22:20:08, Ami Fischman wrote: > (with-temp-file (concat filename ".fontified") > (insert (fontify filename))) https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:83: (dolist (sample samples) (create-golden-sample "media.gyp"))) On 2012/07/19 22:20:08, Ami Fischman wrote: > s/"media.gyp"/sample/ Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:87: ;;(message "Golden sample: %s" (fontify "/Users/jknotten/bin/splitPDF.py")) On 2012/07/19 22:20:08, Ami Fischman wrote: > drop these two Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el File tools/emacs/gyp.el (right): https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el#... tools/emacs/gyp.el:27: (setq python-continuation-offset 2 On 2012/07/27 20:49:12, Scott Byer wrote: > Drive-by: this is vulnerable to a parent mode hook (say, python-mode-hook) > overwriting these values. A fix is to pull setting these variables out into a > function added to gyp-mode-hook, which will get run after any parent mode hooks. Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el#... tools/emacs/gyp.el:54: (cond ((string-match-p "^\\(dependencies\\|export_dependent_settings\\)" section) 'dependencies) On 2012/07/19 22:20:08, Ami Fischman wrote: > You might enjoy regexp-opt Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el#... tools/emacs/gyp.el:55: ((string-match-p "^\\(action\\|sources[!/]?\\|[a-zA-Z0-9]*_sources[!/]?\\|files\\|include_dirs\\|includes\\|libraries\\|inputs\\|outputs\\)$" section) 'sources) On 2012/07/19 22:20:08, Ami Fischman wrote: > 80-cols (here and elsewhere)? Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el#... tools/emacs/gyp.el:134: ;; other none sources Removed. On 2012/07/19 22:20:08, Ami Fischman wrote: > What are these about? https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el#... tools/emacs/gyp.el:143: including the "", and the first group to be the contents of the string. On 2012/07/19 22:20:08, Ami Fischman wrote: > interesting use of "" inside a "-quoted string :) Done. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el#... tools/emacs/gyp.el:147: (set-match-data The reason for setting the match data is because this is how a FUNCTION (See Search-based fontification) when used as a MATCHER communicates the result. It should be possible to write the font lock mode by supplying our own font-lock-fontify-region function, which could avoid setting match data, but then I expect I would have to repeat a lot of what font-lock-default-fontify-region already does, and also make a plan for how to highlight the keywords listed on line 183 and below. I've moved the setting of the match data into gyp-section-match itself, and removed it from gyp-parse-to, which is a good intermediate step if you feel that we should go the full hog and write our own a region fontification function. Not sure what you mean by clobbering non-gyp-specific match data, though? On 2012/07/19 22:20:08, Ami Fischman wrote: > I'm still distressed by your clobbering of the (non-gyp-specific) match-data. > Is it necessary? https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/media.gyp File tools/emacs/media.gyp (right): https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/media.g... tools/emacs/media.gyp:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/07/19 22:20:08, Ami Fischman wrote: > Move this file and its .fontified sibling into a testdata/ dir? Done.
LGTM % nit https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... File tools/emacs/gyp-tests.el (right): https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:40: (read (current-buffer)))) On 2012/08/16 10:40:21, John Knottenbelt wrote: > Unfortunately string= doesn't compare string properties, and the result of > (fontify sample) is a string with associated properties. One possibility is to > use equal-including-properties, but I want to take advantage of ert's > explanation functions which describe exactly where the mismatch is. There is > ert-equal-including-properties, but it is very slow, which is why I have gone > with the list of properties comparison approach (much faster and gives you the > position at which there was a difference). > > On 2012/07/19 22:20:08, Ami Fischman wrote: > > You could replace this read with simply asserting that (fontify sample) > > string='s the golden .fontified string. > I may have been confused earlier. https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp-tes... tools/emacs/gyp-tests.el:44: (find-file "media.gyp") On 2012/08/16 10:40:21, John Knottenbelt wrote: > I wonder if perhaps we shouldn't just remove this test? It tests auto-mode-list > functionality which I think is already pretty solid. What do you think? SGTM https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el File tools/emacs/gyp.el (right): https://chromiumcodereview.appspot.com/10264008/diff/8001/tools/emacs/gyp.el#... tools/emacs/gyp.el:147: (set-match-data On 2012/08/16 10:40:21, John Knottenbelt wrote: > The reason for setting the match data is because this is how a FUNCTION (See > Search-based fontification) when used as a MATCHER communicates the result. It > should be possible to write the font lock mode by supplying our own > font-lock-fontify-region function, which could avoid setting match data, but > then I expect I would have to repeat a lot of what > font-lock-default-fontify-region already does, and also make a plan for how to > highlight the keywords listed on line 183 and below. > > I've moved the setting of the match data into gyp-section-match itself, and > removed it from gyp-parse-to, which is a good intermediate step if you feel that > we should go the full hog and write our own a region fontification function. > > Not sure what you mean by clobbering non-gyp-specific match data, though? > > On 2012/07/19 22:20:08, Ami Fischman wrote: > > I'm still distressed by your clobbering of the (non-gyp-specific) match-data. > > Is it necessary? > I meant that communicating from gyp-mode to font-lock-mode through a variable that belongs to neither (match-data) and is further a core part of how a lot of functionality is built, is frightening to me. But, as you say, this is apparently how font-lock works, and it seems reasonable to me to stick with that approach. https://chromiumcodereview.appspot.com/10264008/diff/18001/tools/emacs/gyp.el File tools/emacs/gyp.el (right): https://chromiumcodereview.appspot.com/10264008/diff/18001/tools/emacs/gyp.el... tools/emacs/gyp.el:159: ;; Set the match data so that it can be manipulated by gyp-section-match comment out of date?
Thanks for your patience, Ami!
LGTM - Thanks for seeing this through!
Added copyright notice to .el and .sh
LGTM
Can you reimplement this as a vim plugin? That'd be much more useful I think. (j/k. I'll land this when I'm in the office.)
hummer:gyp-rw thakis$ tools/emacs/run-unit-tests.sh Cannot open load file: ert.el ^ Is this expected?
Rounding out the review with a couple of messages that didn't make it to the reviewlog: On 2012/08/20 15:33:56, Nico wrote: > hummer:gyp-rw thakis$ tools/emacs/run-unit-tests.sh > Cannot open load file: ert.el > > ^ Is this expected? John wrote: Hi Nico, Yes. You need Emacs 24 to run the tests. You can also supplement them yourself, but easiest is to use Emacs 24, as ert is now part of Emacs. Cheers John Then Nico wrote: Ok, landed blindly in r1474 then :-) (which is http://code.google.com/p/gyp/source/detail?r=1474) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
