Re: [gnu elpa] Adding disk.el
Philip Kaludercic <[email protected]>
| Newsgroups | gmane.emacs.devel |
|---|---|
| Message-ID | <[email protected]> |
Mario Rosell <[email protected]> writes: > Hello. > > A few weeks ago I sent an Emacs patch for adding =disk.el= to the > upstream Emacs dist, since it didn't get accepted, I am now submitting > it as part of GNU Elpa. > > disk.el provides an indent-tabs-mode interface to block devices, plus a > couple commands to manipulate them. > > Repo is on SourceHut: https://git.sr.ht/~rosell/disk.el I also went over the package to collect a few comments: > Since this is my first Elpa package, please tell me if I did something > wrong... (All is fine :) Out of curiosity, have you seen https://elpa.gnu.org/contributing.html?) > Thanks in advance.
(unnamed)
(text/x-patch, 2.9 KB)
diff --git a/disk.el b/disk.el
index ed3aa22..af4d426 100644
--- a/disk.el
+++ b/disk.el
@@ -47,6 +47,9 @@
:type 'boolean
:group 'disk)
+;; For users on systems that don't have this command installed, what
+;; will the error message be and how informative is it if you don't
+;; know much about disk managment?
(defcustom disk-udisksctl-command "udisksctl"
"The path of the udisksctl executable."
:type 'string
@@ -59,11 +62,11 @@
(defun disk--s (v)
"Convert V to a safe string."
- (if (null v) "" (format "%s" v)))
+ (format "%s" (or v "")))
(defun disk--alist-get (key alist)
"Get KEY from ALIST using string comparison."
- (alist-get key alist nil nil #'equal))
+ (cdr (assoc-string key alist)))
(defun disk--flatten (dev)
"Flatten DEV and its children into tabulated-list entries."
@@ -82,6 +85,8 @@
(defun disk--devname (name)
"Ensure NAME has /dev/ prefix."
+ ;; Is this supposed to be file-system agnostic, i.e. just a
+ ;; syntactic check, or should you use `file-in-directory-p'?
(if (string-prefix-p "/dev/" name)
name
(concat "/dev/" name)))
@@ -116,16 +121,19 @@
"-J"
"-o"
"NAME,LABEL,FSTYPE,SIZE,TYPE,MOUNTPOINT"))
- (user-error "Disk: lsblk failed"))
+ (error "Disk: lsblk failed"))
(buffer-string)))
(data (condition-case err
+ ;; You are using the Elisp JSON API here, why not
+ ;; use the native (i.e. faster) API that has been
+ ;; built-in since Emacs 27?
(json-read-from-string json-output)
(error
(message "disk: JSON parse error: %S" err)
nil)))
(devices (disk--alist-get "blockdevices" data))
(entries (mapcan #'disk--flatten devices)))
- (setq tabulated-list-entries entries)
+ (setq tabulated-list-entries entries) ;note that this can also be a function
(tabulated-list-print t)
(message "Disk: loaded %d devices" (length entries))))
@@ -168,7 +176,7 @@
(unless (zerop
(call-process disk-udisksctl-command nil "*disk*" nil
"mount" "-b" (disk--devname dev)))
- (user-error "Disk: udisksctl failed"))
+ (error "Disk: udisksctl failed")) ;perhaps mention the buffer with the error message?
(disk-refresh))))
;;;###autoload
@@ -179,12 +187,16 @@
(if (null dev)
(message "No device selected")
(message "Unmounting %s..." dev)
+ ;; Hiding dead code in comments is always kind of smelly, should
+ ;; it be here or not? Either way, actually using the comment to
+ ;; explain it would be useful.
+ ;;
;; (unless (executable-find disk-udisksctl-command)
;; (user-error "Cannot find udisksctl executable"))
(unless (zerop
(call-process disk-udisksctl-command nil "*disk*" nil
"unmount" "-b" (disk--devname dev)))
- (user-error "Disk: udisksctl failed"))
+ (error "Disk: udisksctl failed"))
(disk-refresh))))
;; Keybindings