Re: Changes to make in elpa-packages file for nongnu elpa

[Thierry Volpiatto]

Hello Philip,

Philip Kaludercic <philipk@posteo.net> writes:

> Thierry Volpiatto <thievol@posteo.net> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Thierry Volpiatto <thievol@posteo.net> writes:
>>>
>>>> I see that Helm installation from nongnu elpa doesn't include
>>>> emacs-helm.sh script, would it be possible to modify this?
>>>>
>>>>         diff --git a/elpa-packages b/elpa-packages
>>>>         index 77d7a5409e..0923724b62 100644
>>>>         --- a/elpa-packages
>>>>         +++ b/elpa-packages
>>>>         @@ -312,11 +312,11 @@
>>>>            :ignored-files "COPYING")
>>>>          
>>>>           (helm                    :url 
>>>> "https://github.com/emacs-helm/helm";
>>>>         -  :ignored-files ("images" "Cask" "Makefile" "emacs-helm.sh" 
>>>> "helm-core.el" "helm.el"
>>>>         +  :ignored-files ("images" "Makefile" "helm-core.el" "helm.el"
>>>>                      "helm-lib.el" "helm-source.el" "helm-multi-match.el"))
>>>>          
>>>>           (helm-core               :url 
>>>> "https://github.com/emacs-helm/helm";
>>>>         -  :ignored-files ("images" "Cask" "Makefile" "emacs-helm.sh" 
>>>> "helm-adaptive.el"
>>>>         +  :ignored-files ("images" "Makefile" "emacs-helm.sh" 
>>>> "helm-adaptive.el"
>>>>                      "helm-bookmark.el" "helm-buffers.el" "helm-color.el" 
>>>> "helm-comint.el"
>>>>                      "helm-command.el" "helm-config.el" "helm-dabbrev.el" 
>>>> "helm-easymenu.el"
>>>>                      "helm-elisp.el" "helm-elisp-package.el" "helm-epa.el" 
>>>> "helm-eshell.el"
>>>
>>> There shouldn't be any issue in applying this patch, but could you
>>> explain the propose of this script?
>>
>> It is used specially for reproducing bugs in a clean environment, see it
>> as emacs -Q for Emacs when reporting bugs. This script starts Emacs -Q
>> with only Helm loaded, this ensure the bug if one comes from Helm and
>> not another package. This is important especially nowaday people are
>> using "Emacs distribution" with the world list of packages installed.
>> Apart that the script is useful to quickly launch Emacs with helm, one
>> can use it from the Helm directory or symlinked to e.g. ~/bin. 
>
> I see.  In that case is there any reason you implement this as a shell
> script?

Well when I wrote the script, packages where not existing and from
outside emacs it is actually the only way to run a package isolated.

> (It might be interesting to provide something like this for
> package.el, to test packages in a generic way.)

Yes, this would be interesting, it would be something like this:

Emacs -Q
M-x <A command that run a package alone, isolated from other
packages nuisances>

> I am sort of an unofficial co-maintainer, without access to any
> server-side infrastructure, but other than that I'm glad to help within
> my possibilities.

Ok thanks.

>> Ok, it's what I thought, so yes it would be nice to install wfnames
>> package in Nongnu.
>> Here is the link: https://github.com/thierryvolpiatto/wfnames
>
> Sure, but could you first take a look at these comments, suggestions and
> questions I have made while skimming over the source?
>
> diff --git a/wfnames.el b/wfnames.el
> index e5a83b5..a61a18d 100644
> --- a/wfnames.el
> +++ b/wfnames.el
> @@ -34,20 +34,20 @@
>  
>  ;; Usage:
>  ;; Once in the Wfnames buffer, edit your filenames and hit C-c C-c to
> -;; save your changes. You have completion on filenames and directories
> +;; save your changes.  You have completion on filenames and directories
>  ;; with TAB but if you are using Iedit package and it is in action use 
> =M-TAB=.
>  
>  ;;; Code:
>  
> -(require 'cl-lib)
> +(eval-when-compile (require 'cl-lib))        ;you are only using macros, so 
> this should be OK

Ok done.
  
>  ;; Internal.
> -(defvar wfnames-buffer "*Wfnames*")
> -(defvar wfnames--modified nil)
> +(defvar wfnames-buffer "*Wfnames*")  ;perhaps `defconst'
> +(defvar-local wfnames--modified nil)

Ok done.

>
>  (defgroup wfnames nil
>    "A mode to edit filenames."
> -  :group 'wfnames)
> +  :group 'wfnames)                   ;watch out, this is a recursive group

Good catch, done.

>  (defcustom wfnames-create-parent-directories t
>    "Create parent directories when non nil."
> @@ -57,16 +57,21 @@
>    "Ask confirmation when overwriting."
>    :type 'boolean)
>  
> -(defvar wfnames-after-commit-hook nil)
> +(defcustom wfnames-after-commit-hook nil
> +  "Hook to run after `wfnames-commit-buffer'." ;rephrase this

Not sure how to rephrase this:

"Hook that run after `wfnames-commit-buffer'."?

> 
> + :type 'hook)

Othewise defcustom make sense, done.

>  (defcustom wfnames-after-commit-function #'kill-buffer
>    "A function to call on `wfnames-buffer' when done."
>    :type 'function)
>  
>  (defcustom wfnames-make-backup nil
> -  "Backup files before overwriting when non nil."
> +  "Non-nil means files are backed up before overwriting."

Ok done.

> 
>    :type 'boolean)
>
> +;; instead of defining new faces and colours, do you think it would be
> +;; possible to inherit from existing faces?

Apart requiring a package just for the faces no, font-lock family
doesn't provide :background faces.

>  (defface wfnames-modified
>      '((t :background "LightBlue" :foreground "black"))
>    "Face used when filename is modified.")
> @@ -104,27 +109,27 @@
>    "Provide filename completion in wfnames buffer."
>    (let ((beg (line-beginning-position))
>          (end (point)))
> +    ;; Does it make sense to extend beyond END to allow completing
> +    ;; file names mid-string?

This one for now I don't know, I transformed your comment with a FIXME.

> 
>      (list beg end #'completion-file-name-table :exit-function (lambda
> (str _status) (when (and (stringp str) (eq (char-after) ?/))
> (delete-char -1))))))
>  
> -(define-derived-mode wfnames-mode
> -    text-mode "wfnames"
> +(define-derived-mode wfnames-mode text-mode "wfnames"
>      "Major mode to edit filenames.

Ok.

>  Special commands:
>  \\{wfnames-mode-map}"
>    (add-hook 'after-change-functions #'wfnames-after-change-hook nil t)
> -  (make-local-variable 'wfnames--modified)
> -  (set (make-local-variable 'completion-at-point-functions) #'wfnames-capf)
> -  (set (make-local-variable 'revert-buffer-function) 
> #'wfnames-revert-changes))
> +  (setq-local completion-at-point-functions #'wfnames-capf)
> +  (setq-local revert-buffer-function #'wfnames-revert-changes))

Yes done.
  
>  (defun wfnames-abort ()
>    "Quit and kill wfnames buffer."
>    (interactive)
> -  (quit-window t))
> +  (quit-window t))                   ;isn't this `kill-buffer-and-window'

Yes, but quit-window is fine as well (I am used to it).
  
>  (defun wfnames-after-change-hook (beg end _len)
>    "Put overlay on current line when modified.
> @@ -137,9 +142,10 @@ Args BEG and END delimit changes on line."
>                 (eol (line-end-position))
>                 (old (get-text-property bol 'old-name))
>                 (new (buffer-substring-no-properties bol eol))
> -               ov face)
> -          (setq face (if (file-exists-p new)
> -                         'wfnames-modified-exists 'wfnames-modified))
> +               (face (if (file-exists-p new)
> +                         'wfnames-modified-exists
> +                    'wfnames-modified))
> +            ov)

Ok.

>            (setq-local wfnames--modified
>                        (cons old (delete old wfnames--modified)))
>            (cl-loop for o in (overlays-in bol eol)
> @@ -184,10 +190,10 @@ When APPEND is specified, append FILES to existing 
> `wfnames-buffer'."
>                                           "* "
>                                           'face 'wfnames-prefix))
>                            "\n"))
> -      (when append (delete-duplicate-lines (point-min) (point-max))))
> +      (when append (delete-duplicate-lines (point-min) (point-max)))) ;this 
> requires Emacs 24.4
>      (unless append
>        ;; Go to beginning of basename on first line.
> -      (while (re-search-forward "/" (line-end-position) t))
> +      (re-search-forward "\\(?:/[^/]*\\)*/" (line-end-position) t)

Ok.

>        (wfnames-mode)
>        (funcall display-fn wfnames-buffer))))
>
> @@ -202,7 +208,7 @@ When APPEND is specified, append FILES to existing 
> `wfnames-buffer'."
>    "Backup FILE."
>    (when wfnames-make-backup
>      (with-current-buffer (find-file-noselect file)
> -      (let ((backup-by-copying t))
> +      (let ((backup-by-copying t))   ;why is this bound?  isn't this
>        a user preference?

Yes removed.

> 
>          (backup-buffer)) (kill-buffer))))
>  
> @@ -252,7 +258,7 @@ When APPEND is specified, append FILES to existing 
> `wfnames-buffer'."
>                                     (let ((basedir (file-name-directory
>                                                     (directory-file-name 
> new))))
>                                       (unless (file-directory-p basedir)
> -                                       (mkdir basedir 'parents))))
> +                                       (make-directory basedir
> 'parents))))

Ok, make sense.

>                                   (if (and ow (wfnames-ask-for-overwrite new))
>                                       ;; Direct overwrite i.e. first loop.
>                                       (progn
> @@ -300,21 +306,22 @@ With a numeric prefix ARG, revert the ARG next lines."
>      (wfnames-revert-current-line-1)
>      (when (eobp) (forward-line -1))
>      (goto-char (line-beginning-position))
> -    (while (re-search-forward "/" (line-end-position) t))))
> +    (re-search-forward "\\(?:/[^/]*\\)*/" (line-end-position) t)))

Ok, same as above.
  
>  (defun wfnames-revert-changes (_ignore-auto _no-confirm)
>    "Revert wfnames buffer to its initial state.
>  
>  This is used as `revert-buffer-function' for `wfnames-mode'."
>    (with-current-buffer wfnames-buffer
> -    (cl-loop for o in (overlays-in (point-min) (point-max))
> -             when (overlay-get o 'hff-changed)
> -             do (delete-overlay o))
> +    (dolist (o (overlays-in (point-min) (point-max)))
> +      (when (overlay-get o 'hff-changed)
> +     (delete-overlay o)))

If you want ;-) done.

>      (goto-char (point-min))
>      (save-excursion
>        (while (not (eobp))
>          (wfnames-revert-current-line-1)))
> -    (while (re-search-forward "/" (line-end-position) t))))
> +    (re-search-forward "\\(?:/[^/]*\\)*/" (line-end-position) t)))
> +

Ok same as above.
  
>  (provide 'wfnames)
>  
>
>
>> Thanks.

Thanks.

-- 
Thierry

signature.asc
Description: PGP signature