[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
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