Matthieu Moy <[EMAIL PROTECTED]> writes:
>>> I think that's a reasonable example of a way to make a back-end
>>> different from the other, while still reusing all of DVC.
>>
>> I disagree; overriding the entire mode function is too heavy-handed.
>> There is nothing that prevents the backend from using an entirely new
>> function, not derived from dvc-diff-mode or diff-mode.
>
> How is this different from your proposals? Just try:
>
> (add-hook 'dvc-diff-mode-hook (lambda () (gnus-summary-mode)))
>
> and you've turned your diff buffer into a Gnus Summary buffer. That's
> lisp, that's life. Nothing prevents you from turning a coffee-machine
> into a grass-cutter, nothing prevents you from shooting yourself in
> the foot.
True. But in this case, it's clear that you are abusing the mechanism;
hooks are intended for minor tweaks, not wholesale overrding.
While a dispatching dvc function _is_ intended for wholesale
overriding, in most cases.
>> In your case, all you did in the overridden function is add more menu
>> entries; the mode hook is a better solution for that.
>
> If you do that cleanly with a hook, you'll have to at least:
>
> * Add the menu to the menubar yourself
I see; you added a new top-level menu, rather than a child of the
current dvc-diff-mode-menu.
That's a choice.
> * Set the keymap yourself
This is a side effect of using define-derived-mode; in a hook, you can
easily add to the current keymap.
> As a reminder, the _whole_ code managing the xgit-diff-mode is this:
>
> (defvar xgit-diff-mode-map
> (let ((map (make-sparse-keymap)))
> (define-key map [?A] 'xgit-status-add-u)
> (define-key map [?R] 'xgit-status-reset-mixed)
> map))
>
> (easy-menu-define xgit-diff-mode-menu xgit-diff-mode-map
> "`Git specific changes' menu."
> `("GIT-Diff"
> ["Re-add modified files (add -u)" xgit-status-add-u t]
> ["Reset index (reset --mixed)" xgit-status-reset-mixed t]
> ))
>
> (define-derived-mode xgit-diff-mode dvc-diff-mode "xgit-diff"
> "Mode redefining a few commands for diff."
> )
>
> (16 lines including the blanks)
I agree that define-derived-mode makes for very clean code in this
case.
I have other code that adds to the Ada-mode menu; it looks like this:
(define-key-after (lookup-key ada-mode-map [menu-bar Ada Project]) [reload]
'("Reload" . ada-prj-reload-project-file) 'Load)
much harder to understand. Although a new define-key helper function
would make it better.
> If you think about it, your solution is to
>
> * Run the code of dvc-diff-mode,
>
> * Run arbitrary code afterwards.
>
> And my solution is to use define-derived-mode to
>
> * Run the code of dvc-diff-mode,
>
> * Run some non-arbitrary code decided by define-derived-mode (keymap, ...)
>
> * Run arbitrary code afterwards.
>
> ;-).
Yes. But there is a difference in documentation of intent.
So maybe if we have a policy that says:
"<back-end>-dvc-diff-mode" should be derived from dvc-diff-mode
(via define-derived-mode), and only extend the menu and keymap
(see xgit.el for a good example).
I could live with that.
--
-- Stephe
_______________________________________________
Dvc-dev mailing list
[email protected]
https://mail.gna.org/listinfo/dvc-dev
