Re: [NonGNU ELPA] New package: hyperdrive (repast)

[Joseph Turner <joseph@ushin.org>]

Philip Kaludercic <philipk@posteo.net> writes:

> Done.  Overall it looks fine, and I'll add the
> package to NonGNU ELPA, it would be nice if you could take a look and
> consider addressing the changes I propose and comments I made:

Thank you for the approval and especially for your code review!!

> diff --git a/hyperdrive-lib.el b/hyperdrive-lib.el
> index 43ca32e3cb..d77b3d1e3c 100644
> --- a/hyperdrive-lib.el
> +++ b/hyperdrive-lib.el
> @@ -1034,6 +1034,8 @@ Affected by option `hyperdrive-reuse-buffers', which see."
>    "Call `message' with MESSAGE and ARGS, prefixing MESSAGE with \"Hyperdrive:\"."
>    (apply #'message (concat "Hyperdrive: " message) args))
>
> +;; Might be nice to use `define-error' for `condition-case' to detect
> +;; this as a specific error type?

I made this change in a separate branch for Adam and I to consider together.

>  (defun hyperdrive-error (string &rest args)
>    "Call `error' with STRING and ARGS, prefixing STRING with \"Hyperdrive:\"."
>    (apply #'error (concat "Hyperdrive: " string) args))
> @@ -1099,6 +1101,8 @@ When BASE is non-nil, PATH will be expanded against BASE instead."
>  ;; of Emacs (going back to the version we declare support for), for
>  ;; features that aren't present in `compat'.
>
> +;; If there is anything you'd like to see in Compat, please mention it.
> +

See <https://github.com/emacs-compat/compat/issues/27>.

>  (eval-and-compile
>    (if (< emacs-major-version 29)
>        (define-derived-mode hyperdrive-clean-mode fundamental-mode "Clean"
> diff --git a/hyperdrive-mirror.el b/hyperdrive-mirror.el
> index caa9e881b7..27113c5052 100644
> --- a/hyperdrive-mirror.el
> +++ b/hyperdrive-mirror.el
> @@ -166,7 +166,7 @@ predicate and set NO-CONFIRM to t."
>  ;;;; Mode
>
>  (defvar-keymap hyperdrive-mirror-mode-map
> -  :parent  tabulated-list-mode-map
> +  :parent tabulated-list-mode-map
>    :doc "Local keymap for `hyperdrive-mirror-mode' buffers."
>    "C-c C-c"   #'hyperdrive-mirror-do-upload)
>
> diff --git a/hyperdrive-vars.el b/hyperdrive-vars.el
> index afc3cf6ef5..ba59e042d9 100644
> --- a/hyperdrive-vars.el
> +++ b/hyperdrive-vars.el
> @@ -39,18 +39,15 @@
>  (defcustom hyperdrive-storage-location
>    (expand-file-name "~/.local/share/hyper-gateway-nodejs/")
>    "Location to store Hypercore data."
> -  :type '(file :must-match t)
> -  :group 'hyperdrive)
> +  :type '(file :must-match t))
>
>  (defcustom hyperdrive-hyper-gateway-port 4973
>    "Port on which to run the hyper-gateway server."
> -  :type 'natnum
> -  :group 'hyperdrive)
> +  :type 'natnum)
>
>  (defcustom hyperdrive-honor-auto-mode-alist t
>    "If non-nil, use file extension of hyperdrive file to set `major-mode'."
> -  :type 'boolean
> -  :group 'hyperdrive)
> +  :type 'boolean)
>
>  (defcustom hyperdrive-persist-location nil
>    "Location where `persist' will store data.
> @@ -58,19 +55,18 @@
>  - `hyperdrive-hyperdrives'
>  - `hyperdrive-version-ranges'"
>    :type '(choice (const :tag "Use default persist location" nil)
> -                 (file :tag "Custom location"))
> -  :group 'hyperdrive)
> -
> -(defcustom hyperdrive-download-directory (expand-file-name
> -                                          (if (bound-and-true-p eww-download-directory)
> -                                              (if (stringp eww-download-directory)
> -                                                  eww-download-directory
> -                                                (funcall eww-download-directory))
> -                                            "~/"))
> +                 (file :tag "Custom location")))
> +
> +(defcustom hyperdrive-download-directory
> +  (expand-file-name
> +   (if (bound-and-true-p eww-download-directory)
> +       (if (stringp eww-download-directory)
> +           eww-download-directory
> +         (funcall eww-download-directory))
> +     "~/"))
>    "Location where `hyperdrive-download-url' will download files.
>  Defaults to `eww-download-directory'."
> -  :type '(file :must-match t)
> -  :group 'hyperdrive)
> +  :type '(file :must-match t))
>
>  (defvar hyperdrive-timestamp-format-string)
>  (defcustom hyperdrive-timestamp-format "%x %X"
> @@ -78,20 +74,20 @@ Defaults to `eww-download-directory'."
>  Passed to `format-time-string', which see."
>    :type 'string
>    :set (lambda (option value)
> -         (set option value)
> +         (set-default option value)
>           (setf hyperdrive-timestamp-format-string
>                 (format "%%%ds"
>                         ;; FIXME: This value varies based on current
>                         ;;        time. (format-time-string "%-I") will
>                         ;;        be one or two characters long
>                         ;;        depending on the time of day
> -                       (string-width (format-time-string value)))))
> -  :group 'hyperdrive)
> +                       (string-width (format-time-string value))))))
>
>  (defcustom hyperdrive-directory-display-buffer-action
>    '(display-buffer-same-window)
>    "Display buffer action for hyperdrive directories.
>  Passed to `display-buffer', which see."
> +  ;; Perhaps use `display-buffer--action-custom-type'?
>    :type '(choice (const :tag "Same window" (display-buffer-same-window))
>                   (const :tag "Pop up window" (display-buffer-pop-up-window))
>                   (sexp :tag "Other"))
> @@ -103,13 +99,11 @@ Passed to `display-buffer', which see."
>  Passed to `display-buffer', which see."
>    :type '(choice (const :tag "Same window" (display-buffer-same-window))
>                   (const :tag "Pop up window" (display-buffer-pop-up-window))
> -                 (sexp :tag "Other"))
> -  :group 'hyperdrive)
> +                 (sexp :tag "Other")))
>
> -(defcustom hyperdrive-column-headers 't
> +(defcustom hyperdrive-column-headers t
>    "Display column headers in `hyperdrive-dir' and `hyperdrive-history' buffers."
> -  :type 'boolean
> -  :group 'hyperdrive)
> +  :type 'boolean)
>
>  (defcustom hyperdrive-default-host-format
>    '(petname nickname domain seed short-key public-key)
> @@ -125,8 +119,7 @@ used."
>                    (const :tag "DNSLink domain" domain)
>                    (const :tag "Seed" seed)
>                    (const :tag "Shortened public key" short-key)
> -                  (const :tag "Full public key" public-key)))
> -  :group 'hyperdrive)
> +                  (const :tag "Full public key" public-key))))
>
>  (defcustom hyperdrive-stream-player-command "mpv --force-window=immediate %s"
>    "Command used to play streamable URLs externally.
> @@ -135,19 +128,17 @@ quoted, because the arguments are passed directly rather than
>  through a shell)."
>    :type '(choice (const :tag "MPV" "mpv --force-window=immediate %s")
>                   (const :tag "VLC" "vlc %s")
> -                 (string :tag "Other command"))
> -  :group 'hyperdrive)
> +                 (string :tag "Other command")))
>
>  (defcustom hyperdrive-queue-size 2
>    "Default size of request queues."
>    ;; TODO: Use this elsewhere also.
> -  :type 'integer
> -  :group 'hyperdrive)
> +  :type 'integer)			;natnum?
> +
>
>  (defcustom hyperdrive-render-html t
>    "Render HTML hyperdrive files with EWW."
> -  :type 'boolean
> -  :group 'hyperdrive)
> +  :type 'boolean)
>
>  (defcustom hyperdrive-reuse-buffers 'any-version
>    "How to reuse buffers when showing entries.
> diff --git a/hyperdrive.el b/hyperdrive.el
> index f3024417ae..c94da62fb0 100644
> --- a/hyperdrive.el
> +++ b/hyperdrive.el
> @@ -2,9 +2,9 @@
>
>  ;; Copyright (C) 2022 Joseph Turner <joseph@ushin.org>
>
> -;; Author: Joseph Turner
> +;; Author: Joseph Turner <joseph@ushin.org>
>  ;; Author: Adam Porter <adam@alphapapa.net>
> -;; Maintainer: Joseph Turner <joseph@ushin.org>
> +;; Maintainer: Joseph Turner <~ushin/ushin@lists.sr.ht>
>  ;; Created: 2022
>  ;; Version: 0.2-pre
>  ;; Package-Requires: ((emacs "27.1") (map "3.0") (compat "29.1.4.0") (plz "0.7") (persist "0.5"))

I committed these remaining changes to master and attributed authorship
to you.

Thank you!!

Joseph