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

[Philip Kaludercic <philipk@posteo.net>]

Thierry Volpiatto <thievol@posteo.net> writes:

> 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 was actually thinking of a command like

M-x package-isolate RET foo,bar,baz RET

and a new instance of Emacs using -Q is spun up, with all the packages
you have listed loaded, and nothing else... Sounds like a fun little
weekend project ;^)

>> 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.
>
>>  \f
>>  (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'."?

If it is OK the way it is, then you can keep it, I just wasn't sure if
you would have a more descriptive name for it.

>> + :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)
>>  \f
>> +;; 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.

I wasn't thinking just of font-lock, you could pick any face,
eg. wfnames-modified could inherit from diff-refine-changed.  That way
themes don't have to add extra support, and the package looks more
"native" without any additional effort.

>>  (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.

That is fine, this is just a general thing I wonder about with CAP
functions.

>> 
>>      (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))))
>>  \f
>> @@ -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.

I am totally indifferent, they expand to almost the same code anyway:

--8<---------------cut here---------------start------------->8---
(disassemble
 (byte-compile
  '(cl-loop for o in olist
	    when (foo)
	    do (bar))))

byte code:
  args: nil
0	varref	  olist
1	constant  nil
2:1	stack-ref 1
3	consp	  
4	goto-if-nil 3
7	discard	  
8	dup	  
9	car	  
10	constant  foo
11	call	  0
12	goto-if-nil 2
15	constant  bar
16	call	  0
17	discard	  
18:2	stack-ref 1
19	cdr	  
20	stack-set 2
22	goto	  1
25:3	constant  nil
26	return	  


(disassemble
 (byte-compile
  '(dolist (o olist)
     (when (foo)
       (bar)))))

byte code:
  args: nil
0	varref	  olist
1:1	dup	  
2	goto-if-nil-else-pop 3
5	dup	  
6	car	  
7	discard	  
8	constant  nil
9	constant  foo
10	call	  0
11	goto-if-nil 2
14	constant  bar
15	call	  0
16	discard	  
17:2	discard	  
18	cdr	  
19	goto	  1
22:3	return	  
--8<---------------cut here---------------end--------------->8---


>>      (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.

Great, I'll add the package then.