[bug#40928] [PATCH 1/2] gnu: Add emacs-ryo-modal.

[Nicolas Goaziou]

Hello,

pinoaffe <address@hidden> writes:

> * gnu/packages/emacs-xyz.scm (emacs-ryo-modal): New variable.

Thank you. 

I have some comments about the patch.

> +(define-public emacs-ryo-modal
> +  (package
> +    (name "emacs-ryo-modal")
> +    (version "20191017.1323")

This version comes from MELPA, but is not official. I suggest to use
"0.4", as defined in the main file, with a "revision" binding and
a "commit" reference. See, e.g., `emacs-uml-mode'.

> +    (source
> +      (origin
> +        (method url-fetch)
> +        (uri (string-append
> +               "https://melpa.org/packages/ryo-modal-";
> +               version
> +               ".el"))

We do not use files from MELPA, as those are not stable, IIRC. You may
want to git-fetch from GitHub directly. Again, see, e.g.,
`emacs-uml-mode'.

> +        (sha256
> +          (base32
> +            "0hslayyfckhirx9sk1pjdrl7acgvwj89aa5spiavz4z6a8rm17js"))))

Nitpick: I suggest to put `base32' on the same line as the hash.

> +    (build-system emacs-build-system)
> +    (home-page
> +      "http://github.com/Kungsgeten/ryo-modal";)

This could go on the same line.

> +    (synopsis "Roll your own modal mode")

Could you expound the synopsis a bit? Maybe:

  Emacs minor mode for defining your own modal editing environment

> +    (description
> +      "ryo-modal provides a convenient way of defining modal keybindings in 
> Emacs.

I suggest:

  RYO modal provides a way of defining...

> +The primary way of binding keys is using `ryo-modal-key' and 
> `ryo-modal-keys'.
> +Both of these functions provide useful keyword arguments.
> +`ryo-modal-mode' is used to toggle the modal editing environment.
> +ryo-modal does not come with any predefined bindings!

IMO the description should not be a manual. Could you remove references
to variables and functions?

> +If you want bindings that only should be active when the region is active,
> +please have a look at `selected-minor-mode'
> (https://github.com/Kungsgeten/selected.el).")

I don't think this is needed in the description either.

Could you send an updated patch?

Regards,

-- 
Nicolas Goaziou