[emacs-w3m:13096] Re: A git-ish patch for w3m-db-history

On 30/01/2019 13:00 -0500, Boruch Baum wrote:

> On 2019-01-30 19:50, Filipp Gunbin wrote:
>> A few comments:
>
> Thanks for the feedback. Did you try the code to verify that it
> operates?

Nope :-)  I've just looked through it.

>> > +; ARGS is not used. It is necessary in order to >/dev/null unnecessary
>>
>> Usually full-line comments start with two semicolons.
>>
>> >  (defun w3m-db-history (&optional start size)
>> [..]
>> > +  (interactive)
>> > +  (cond
>> > +   ((or executing-kbd-macro noninteractive)
>> > +    (when (not start) (setq start 0))
>> > +    (when (not size)  (setq size w3m-db-history-display-size)))
>> > +   (t ; called interactively; possibly indirectly
>> > +    (setq start (read-number "start: " (or w3m-db-history-display-size 0)))
>> > +    (setq size (read-number "size: " (or size 0)))))
>>
>> It's easier to read code when you have normal interactive form: that is,
>> which either takes string arg-descriptor, or a list which may contain
>> calls like read-something.  Reading anywhere outside this (interactive
>> (list ...)) looks awkward.
>
> It may look awkward because its an awkward situation, or because I'm not
> a certified expert elisp programmer, so I'll welcome any suggestions to
> address all the constraints of the circumstances:
>
> 1) Handle a truly non-interactive call.
>
> 2) Handle a kind-of non-interactive call, like when it's called by
>    function w3m-history. This is expected to be the most common
>    actual-use case, because we have a default keybinding for function
>    w3m-history and we are already setting very many keybindings.
>
> 3) Handle a direct interactive call.
>
> 4) Perform input validation on the arguments to ensure they are
>    integers.

All of this can be handled in rather standard way: each interactive
function reads its own input in "interactive" form.

If you have complex arg-reading code which you would like to share
between various commands, then create a function which does it and
invoke it from (interactive) form instead of (list) - I didn't do that
myself, but it should be straightforward.

For case 2, if w3m-history doesn't read itself enough arguments for your
new function, it could be modified to do that.

Any validation and default-setting comes after (interactive) form, so it
is applied both to interactive and non-interactive calls.

HTH, Filipp