From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: Re-including rst.el into Emacs repository Date: Mon, 07 May 2012 21:42:04 -0400 Message-ID: References: <30996.1335792548@eskebo.merten-home.homelinux.org> <7259.1336421206@theowa.merten-home.homelinux.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: dough.gmane.org 1336441339 13368 80.91.229.3 (8 May 2012 01:42:19 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Tue, 8 May 2012 01:42:19 +0000 (UTC) Cc: emacs-devel@gnu.org To: Stefan Merten Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue May 08 03:42:18 2012 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1SRZRQ-0002bm-6P for ged-emacs-devel@m.gmane.org; Tue, 08 May 2012 03:42:16 +0200 Original-Received: from localhost ([::1]:42276 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SRZRP-0003GV-Kr for ged-emacs-devel@m.gmane.org; Mon, 07 May 2012 21:42:15 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:43400) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SRZRK-00036i-S1 for emacs-devel@gnu.org; Mon, 07 May 2012 21:42:12 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SRZRI-0000I4-2Z for emacs-devel@gnu.org; Mon, 07 May 2012 21:42:10 -0400 Original-Received: from ironport-out.teksavvy.com ([206.248.143.162]:3755) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SRZRH-0000Hf-SQ for emacs-devel@gnu.org; Mon, 07 May 2012 21:42:08 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ApYIACxOgk9FxLCA/2dsb2JhbABDuCMDgQyBCIIJAQEEAVYjBQsLDiYSFA0LDSQTGodmAwYFtDoNgWuKLXs5hHkEoSWDIIFdgwOBOAEW X-IronPort-AV: E=Sophos;i="4.75,391,1330923600"; d="scan'208";a="178867670" Original-Received: from 69-196-176-128.dsl.teksavvy.com (HELO pastel.home) ([69.196.176.128]) by ironport2-out.teksavvy.com with ESMTP/TLS/ADH-AES256-SHA; 07 May 2012 21:42:05 -0400 Original-Received: by pastel.home (Postfix, from userid 20848) id 9866B596D6; Mon, 7 May 2012 21:42:04 -0400 (EDT) In-Reply-To: <7259.1336421206@theowa.merten-home.homelinux.org> (Stefan Merten's message of "Mon, 07 May 2012 22:06:46 +0200") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 206.248.143.162 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:150364 Archived-At: > First step is done :-) . Thanks. > Just committed. I'd appreciate a thorough review. See some comments below (the patch is much too large to review in detail, and I trust you on the meat of the code, concentrating on nitpicks instead). >> But please try to keep some reference to the >> "common ancestor" and "tip" of the branch being merged (that's done >> automatically as Bzr metadata when it's a normal merge, but I suspect in >> your case the branch from which you merge is external). > I did this in the log message including a reference to the Docutils > subversion revision. Thanks, tho you only put the tip of the merge, and forgot its base. > I worked on this and think I ended up in a pretty good result. I used > my result as a log message. Looks OK (see comments below), but next time please include the Changelog both as commit message and in the ChangeLog file. Obviously it's redundant and hopefully we will be able to get rid of this redundancy at some point, but for now this is how we do it. > I did this in the same commit. I hope I did it right. Doing it in the same commit is good, thanks. Stefan > 2012-05-05 Stefan Merten > > * rst.el: Major merge with upstream development up to Docutils > SVN r7399 / rst.el V1.2.1. This format is exactly right for the ChangeLog file, but for the commit message, you don't need the first line (the date and author are already recorded as other metadata), and you don't need the leading TABs on each line. If you write the ChangeLog file first, and commit from Emacs, you can use C-c C-a in the *vc-log* buffer to let Emacs fetch the message from the ChangeLog file and pull it into the *vc-log* message, removing the TABs and moving any author/bug information into appropriate header fields. > Clarified maintainership and authors. If you check the ChangeLog guidelines, they recommend the use of the present tense: describe what the change does. I.e., here I'd say "Clarify maintainership and authors". > (rst-re): New function for reStructuredText regexes. Used in > many places. Same here (and at many other places) for use of the past tense. Also, we use two spaces after "." (see `sentence-end-double-space'), as is standard in the US. > (rst-font-lock-handle-adornment-matcher): Major revision of > font-locking. Integrated with other code. `jit-lock-mode' is > used now. Same here (double spaces, past). Additionally, try and use the active form and don't bother saying "now" since the whole message is about what is happening "now" anyway: "Use jit-lock-mode". > (rst-compile): Shell arguments are quoted. Here, I'd say "Quote shell arguments". > (rst-compile-pdf-preview, rst-compile-slides-preview): > Temporary files are deleted after use. And here "Delete temporary files after use". > +** reStructuredText mode > + > +*** Major merge with upstream development. We usually don't mention this and concentrate on the actual visible changes. > +*** Nearly all keys are rebound making room for more keys and comply > +better to usage in other modes. Bindings are described with C-c C-h. I'm not very good at writing NEWS entries either, so I won't try to improve them for you here, but you're using the passive voice and a single space after the ".". > +*** Major revision of fontification. Now works with `jit-lock-mode'. > +Thanks to Stefan Monnier for help. Don't put acknowledgments in the NEWS file. > -;; Authors: Martin Blais , > -;; Stefan Merten , > -;; David Goodger > +;; Maintainer: Stefan Merten > +;; Author: Martin Blais , > +;; David Goodger , > +;; Wei-Wei Guo Thanks for clarifying. BTW, usually the Authors list is exhaustive (i.e. includes the maintainer). It doesn't matter much in practice, but it's convenient to be able to change the maintainer without having to worry about whether it's already on the Authors list or not. Also in theory the maintainer might not actually be an author (e.g. if she limits her contribution to choosing which patches to accept). > +;; This package provides major mode rst-mode, which supports documents marked > +;; up using the reStructuredText format. Support includes font locking as well > +;; as a lot of convenience functions for editing. It does this by defining a > +;; Emacs major mode: rst-mode (ReST). This mode is derived from text-mode. This > +;; package also contains: If you try C-u checkdoc-current-buffer, it will point out the (mis)use of single space after ".". > ;; ("\\.rest$" . rst-mode)) auto-mode-alist)) You didn't change it, but I'll point out that the above should be "\\.rest\\'" (in the unlikely event of a file named "foo.rest\ntoto.c" ;-). > +(require 'cl) For various reasons some of the Emacs maintainers don't want to force CL to be loaded (mostly issues of name space uncleanliness), so please try to limit your use of CL to macros and use (eval-when-compile (require 'cl)). Hopefully, this problem will be lifted soon (with the move from `cl' to `cl-lib' where all the definition will be properly prefixed by "cl-"), but don't hold your breath. > +(defun rst-extract-version (delim-re head-re re tail-re var &optional default) > + "Return the version matching RE after regex DELIM-RE and HEAD-RE > +and before TAIL-RE and DELIM-RE in VAR or DEFAULT for no match" As C-u M-x checkdoc-current-buffer will point out, the first line of a docstring should stand on its own, so that things like "apropos" (where only the first line is displayed) make sense. Not sure what would be best, but maybe something like "Return the string matched by RE within a given context. The string matching RE should be preceded by text matching DELIM-RE and HEAD-RE and should be followed by text matching TAIL-RE and DELIM-RE." > + (if (string-match > + (concat delim-re head-re "\\(" re "\\)" tail-re delim-re) > + var) > + (match-string 1 var) > + default)) Tho, you could also just get rid of the docstring altogether, since the code is just as clear (especially if you wanted to make the docstring precise enough, e.g. specify that neither DELIM-RE nor HEAD-RE should include a non-shy subgroup). Also I do not understand why the string argument is called `var', which seems counter-intuitive since it won't hold a variable but a string and that string doesn't describe any kind of variable either. > +;; FIXME: Use `sregex` or `rx` instead of re-inventing the wheel How 'bout fixing this one? > + (let (rst-re-alist) > + (dolist (re rst-re-alist-def) > + (setq rst-re-alist > + (nconc rst-re-alist > + (list (list (car re) (apply 'rst-re (cdr re))))))) > + rst-re-alist) Not that it matters, but this is an O(N^2) algorithm which you can turn into a O(N) algorithm by using (push (list (car re) (apply 'rst-re (cdr re)))) rst-re-alist) and a final nreverse. That's a classic "Elisp code pattern". > +(defun rst-call-deprecated () > + (interactive) > + (let* ((dep-key (this-command-keys-vector)) > + (dep-key-s (format-kbd-macro dep-key)) > + (fnd (assoc dep-key rst-deprecated-keys))) > + (if (not fnd) > + ;; Exact key sequence not found. Maybe a deprecated key sequence has > + ;; been followed by another key. > + (let* ((dep-key-pfx (butlast (append dep-key nil) 1)) > + (dep-key-def (vconcat dep-key-pfx '(t))) > + (fnd-def (assoc dep-key-def rst-deprecated-keys))) > + (if (not fnd-def) > + (error "Unknown deprecated key sequence %s" dep-key-s) > + ;; Don't execute the command in this case > + (message "[Deprecated use of key %s; use key %s instead]" > + (format-kbd-macro dep-key-pfx) > + (format-kbd-macro (second fnd-def))))) > + (message "[Deprecated use of key %s; use key %s instead]" > + dep-key-s (format-kbd-macro (second fnd))) > + (call-interactively (third fnd))))) This (this-command-keys-vector) business is pretty messy. I think I'd have defined instead new commands by replacing: > + (dolist (dep-key deprecated) > + (push (list dep-key key def) rst-deprecated-keys) > + (define-key keymap dep-key 'rst-call-deprecated))) with something like: (dolist (dep-key deprecated) (define-key keymap dep-key `(lambda () ,(format "Deprecated binding for %s, use \\[%s] instead." def def) (let ((adv-binding (where-is-internal ',def nil t))) (when adv-binding (message "[Deprecated use of key %s; use key %s instead]" (key-description (this-command-keys)) (key-description adv-binding)))) (call-interactively ',def)))) Tho maybe even better would be to emit the message after running the command, otherwise the message might not be visible if the command emits its own message(s). > + ;; same as mark-defun sgml-mark-current-element Please treat your comments with respect: capitalize the first word and terminate it with proper punctuation. > (defvar rst-mode-abbrev-table nil > - "Abbrev table used while in Rst mode.") > + "Abbrev table used while in `rst-mode'.") > (define-abbrev-table 'rst-mode-abbrev-table > (mapcar (lambda (x) (append x '(nil 0 system))) > '(("contents" ".. contents::\n..\n ") Since Emacs-23, you can merge the defvar and the define-abbrev-table. > + (set (make-local-variable 'comment-insert-comment-function) > + 'rst-comment-insert-comment) Please add a comment why you need this (probably in the definition of rst-comment-insert-comment rather than here). > + (set (make-local-variable 'comment-region-function) > + 'rst-comment-region) > + (set (make-local-variable 'uncomment-region-function) > + 'rst-uncomment-region) Same here. > ;; Font lock > - (set (make-local-variable 'font-lock-defaults) > - '(rst-font-lock-keywords-function > + (setq font-lock-defaults > + '(rst-font-lock-keywords On Emacsen that do not pre-load font-lock, this code can set the global value of font-lock-defaults :-( Stefan