Re: Ambrose Kofi Laing & Ralph Neelante Amissah [Emacs] sisu-mode.el - a major-mode for highlighting a structured text

[Stefan Monnier]

> Please find attached sisu-mode.diff for sisu-mode.el

Thanks.  I was about to apply it, but I think it's not quite right.
it's a bit hard to see what the patch does because it includes a lot of
whitespace changes, but even after accounting for those, there are some
changes which I don't think really want to install.  See below.


        Stefan


> -;;; sisu-mode.el --- Major mode for SiSU markup text
> +;;; sisu-mode.el --- a major-mode for highlighting a hierarchy structured 
> text.

Why did you remove SiSU from the description?
[ Not opposed to it, but rather curious.  ]

> -;; Copyright (C) 2011  Free Software Foundation, Inc.
> +;; Copyright (C): Free Software Foundation, Inc. (FSF) (GNU EMACS)

We want to keep all the years of publication in this line.

> -;; Author: Ambrose Kofi Laing (& Ralph Amissah)
> -;; Keywords: text, processes, tools
> -;; Version: 3.0.3
> +;; Author: Ralph Amissah & Ambrose Kofi Laing
> +;; Keywords: text, syntax, processes, tools
> +;; Version:   7.1.7 2015-12-26 Ralph Amissah,

The Version: pseudo-header should be of the form AA.BB.CC.DD... only.

> +;; URL: 
> [http://git.sisudoc.org/gitweb/?p=code/sisu.git;a=blob;f=data/sisu/conf/editor-syntax-etc/emacs/sisu-mode.el;hb=HEAD]

Better than a link to the file would be a link to some "sisu-mode
project" web-page with further information than what's in this file.

>  ;; License: GPLv3

This is redundant.

> +      (cons "^group\{\\|^\}group"       'general-font-lock-red2)
                      ^     ^
These two backslashes do not do anything.

> +;; enables outlining for sisu
> +(add-hook 'sisu-mode-hook
> +       '(lambda ()
> +         (outline-minor-mode)))

Please don't quote your lambdas.
Also (lambda () (outline-minor-mode)) is just a round-about way to say
#'outline-minor-mode.
Finally, it's generally preferred to put such code directly in the
sisu-mode function and leave the sisu-mode-hook nil by default.

> +;(define-key evil-normal-state-map (kbd ",0") (lambda() (interactive) 
> (show-all)))

Single-semicolon comments are traditionally indented to comment-column
(e.g. column 40 or so), so you should probably use ";;" instead here.

> -;;;###autoload
> +;; Sisu & Autoload:
>  (define-derived-mode sisu-mode text-mode "SiSU"

This ";;;###autoload" comment is the magic cookie used to auto-generate
sisu-mode-autoloads.el, so you don't want to remove it.

> -  "Major mode for editing SiSU files.
> -SiSU (http://www.sisudoc.org/) is a document structuring and
> -publishing framework.  This major mode handles SiSU markup."
> +  "Major mode for editing SiSU files."
> +  (interactive)

`define-derived-mode` already declares the function as interactive.

>    (run-hooks 'sisu-mode-hook))

And `define-derived-mode` also automatically runs sisu-mode-hook for
you, so you don't need this either.

> -;;;###autoload (add-to-list 'auto-mode-alist '("\\.sisu\\'" . sisu-mode))
> +;; ##autoload
> +(add-to-list 'auto-mode-alist '("\\.sst\\'" . sisu-mode))
> +(add-to-list 'auto-mode-alist '("\\.ssm\\'" . sisu-mode))
> +(add-to-list 'auto-mode-alist '("\\.ssi\\'" . sisu-mode))

I think you want to use something like

   ;;;###autoload
   (add-to-list 'auto-mode-alist '("\\.ss[itm]\\'" . sisu-mode))

or

   ;;;###autoload
   (add-to-list 'auto-mode-alist '("\\.sst\\'" . sisu-mode))
   ;;;###autoload
   (add-to-list 'auto-mode-alist '("\\.ssm\\'" . sisu-mode))
   ;;;###autoload
   (add-to-list 'auto-mode-alist '("\\.ssi\\'" . sisu-mode))

> +;; 2011-07-12  Chong Yidong  <address@hidden>
> +;;
> +;; Fix version numbers of sisu-mode, register-list, and windresize.
> +;;
> +;; 2011-07-08  Chong Yidong  <address@hidden>
> +;;
> +;; sisu-mode.el: Add .sisu to auto-mode-alist using autoload cookie.
> +;; Minor doc fixes.
> +;;
> +;; 2011-07-06  Stefan Monnier  <address@hidden>
> +;;
> +;; * sisu-mode.el (sisu-mode): Autoload.
> +;;
> +;; 2011-07-04  Stefan Monnier  <address@hidden>
> +;;
> +;; Add sisu-mode.el.  Update all.el licence.

This gets auto-added (from the Git log) when generating the ELPA
package, so we don't need it.


        Stefan