[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Nicolas Goaziou
Subject: [bug#40928] [PATCH 1/2] gnu: Add emacs-ryo-modal.
Date: Tue, 28 Apr 2020 17:46:29 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux)


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
> +               "";
> +               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.,

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

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

> +    (build-system emacs-build-system)
> +    (home-page
> +      "";)

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'
> (")

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

Could you send an updated patch?


Nicolas Goaziou

reply via email to

[Prev in Thread] Current Thread [Next in Thread]