勝手に添削 perlbrew.el

emacs

pull requestにしようかとも思ったんですけど、バグフィックスって
わけでもないし、ここに書くことにします。あと多岐にわたるっていう
のもあって、分割して requestするのが面倒だっていうのもいくらか
あります。


私自身 にわか Emacs Lispプログラマなので間違いがあれば指摘してください。

リポジトリ

参考

defgroup

オリジナル
(defgroup perlbrew nil
  "perlbrew"
  :group 'perlbrew)


defgroupのパラメータは defcustomと同じですが、:groupはそれが
所属するグループです。なので上記のようにすると今定義する
グループに所属するということになってしまいます。親グループを
指定するというイメージでしょうか。


何が妥当なのかは悩ましいですが、languagesとか shellとかですかね。
perlでもいいかもしれないんですが、perlだと perl-modeの定義する
グループになってしまい、何か違うように思えます。cperlも同様

perlbrew-use, perlbrew-switch

オリジナル
(defun perlbrew-use (version)
  (interactive (list (completing-read "Version: " (perlbrew-list))))
  (setq perlbrew-perls-dir (concat perlbrew-dir "/perls"))

  (cond ((equal version "system")
         (perlbrew-clean-exec-path)
         (setq perlbrew-current-perl-path
               (perlbrew-trim (shell-command-to-string "which perl"))))
        (t
         (perlbrew-set-current-perl-path version)
         (perlbrew-set-current-exec-path))))

(defun perlbrew-switch (version)
  (interactive (list (completing-read "Version: " (perlbrew-list))))
  (perlbrew-use version))


初めに気になるのは completing-readです。perlbrew-listの中から候補を選べるわけ
ですが、これではでたらめな文字列でも入力することができます。hogehogeでも OKで
結果でたらめなディレクトリに PATHが設定されてしまうことになります。
completing-readの第四引数を真にすると補完候補にマッチすることを強制
できるので、この場合はそうするとよいでしょう。これは perlbrew-switchも同様です。


しかしそうしてもそれが有効に働くのは interactiveに実行されたときだけ
なので、関数呼び出しの際も有効にしたいところです。interactiveでない場合は
引数 versionが perlbrew-listに含まれるかを確認するとよいでしょう。


次に気になるのは perlのパスを知るために whichを使っているところです。
perlbrewを Windowsで使うとしても Unixコマンドが必要なので問題ないかも
しれないですが、これだと whichコマンドがない環境だと失敗してしまいます。
Emacsでは同等の機能を実現するための executable-find関数が存在するので、
素直にそちらを使う方が良いでしょう。


これらのことを踏まえて修正を行うと以下のようになるかと思います。

修正版
(defun perlbrew-use (version)
  (interactive (list (completing-read "Version: " (perlbrew-list) nil t)))
  (setq perlbrew-perls-dir (concat perlbrew-dir "/perls"))
  (unless (called-interactively-p 'interactive)
    (unless (member version (perlbrew-list))
      (error "Not installed version: %s" version)))
  (cond ((equal version "system")
         (perlbrew-clean-exec-path)
         (setq perlbrew-current-perl-path (executable-find "perl")))
        (t
         (perlbrew-set-current-perl-path version)
         (perlbrew-set-current-exec-path))))

(defun perlbrew-switch (version)
  (interactive (list (completing-read "Version: " (perlbrew-list) nil t)))
  (perlbrew-use version))

perlbrew-list

オリジナル
(defun perlbrew-list ()
  (let* ((perls (split-string (perlbrew "list"))))
    (remove-if
     (lambda (i)
       (not (string-match "^\\(perl\\|[0-9]\\|system\\)" i)))
     (append perls '("system")))))


remove-ifで第一引数の perdicate関数で notを取っています。remove-ifの類の
関数は -notを末尾につける関数も用意されています。remove-ifの場合、
remove-if-notです。


remove-ifだと「何かを取り除く」ということを頭に入れてその先を読むかと
思うのですが、predicate中で notがあると頭が混乱します。初めから「何かを残す」と
考えるためにも, remove-if-notを使った方がいいかと思います。


あと "system"を appendしておいて、それが正規表現に含まれるというのも
気になります。そうするなら後で足せば良さそうです。let*もバインドされる
変数間に依存がなければ letの方がいいと思います。上記のことを考えて
修正すると以下のような感じでしょうか。

修正版 1
(defun perlbrew-list ()
  (let* ((perls (split-string (perlbrew "list")))
         (valid-perls (remove-if-not
                       (lambda (i)
                         (string-match "^\\(perl\\|[0-9]\\)" i))
                       perls)))
    (append valid-perls '("system"))))


remove-if-notは cl関数なので byte-compile時に警告が出ます。
それが好ましくないとなると loopを使う以下のようになるでしょうか。

修正版 2
(defun perlbrew-list ()
  (loop for perl in (split-string (perlbrew "list"))
        when (string-match "^\\(perl\\|[0-9]\\)" perl)
        collect perl into valid-perls
        finally
        return (append valid-perls '("system"))))

perlbrew-get-current-perl-path

使われていない。interactiveでもないので、コマンドとして呼ぶことも
ないようなので不要なのではないでしょうか。

perlbrew-join, perlbrew-trim

オリジナル
(defun perlbrew-join (list)
  (mapconcat 'identity list " "))

(defun perlbrew-trim (string)
  (replace-regexp-in-string "\n+$" "" string))

引数の名前が気になります。それぞれ同名の関数があるからです。
listは lst, stringは strでいいのではないでしょうか。strは
ともかく lstは慣習的にそうなので、そうした方がいいと思います。

修正版
(defun perlbrew-join (lst)
  (mapconcat 'identity lst " "))

(defun perlbrew-trim (str)
  (replace-regexp-in-string "\n+$" "" str))

perlbrew-remove-all-perl-path

オリジナル
(defun perlbrew-remove-all-perl-path (path-list)
  (remove-if
   (lambda (i)
     (string-match (concat "^" perlbrew-perls-dir) i))
   path-list))


string-matchの正規表現は一度評価するだけで良いので、外に出した
方がよいでしょう。

修正版 1
(defun perlbrew-remove-all-perl-path (path-list)
  (let ((perlbrew-perl-regexp (format "^" perlbrew-perls-dir)))
    (remove-if (lambda (path)
                 (string-match perlbrew-perl-regexp path))
               path-list)))


上記の byte-compile時の警告を避けるため loopマクロを使う
なら以下のようになります。

修正版 2
(defun perlbrew-remove-all-perl-path (path-list)
  (loop with perlbrew-perl-regexp = (format "^%s" perlbrew-perls-dir)
        for path in path-list
        unless (string-match perlbrew-perl-regexp path)
        collect path))

補足 byte-compile時の警告を無効化

cl関数を使わず、マクロでなんとかすることで、警告は回避できますが、
読みやすさを犠牲にすることがあります。remove-if-notはともかく、
remove-ifだと unless + collectになってしまい、直感的でなくなって
しまう感じがあるので、置き換えることをしたくない場合は、以下の
ようにバッファローカル変数宣言をファイルの末尾に足すとよいでしょう。

;; Local Variables:
;; byte-compile-warnings: (not cl-functions)
;; End: