We have several times run into the issue of what belongs in the DVC
front-end, and what belongs in the back-end. I'd like to address that
directly.
As an example, let's consider dvc-status. Currently, it looks like this:
dvc-unified.el:
(defun dvc-status (&optional path)
"Display the status in optional PATH tree."
(interactive)
(let* ((path (when path (expand-file-name path)))
(default-directory (or path default-directory)))
;; this should be done in back-ends, so that
;; M-x <back-end>-status RET also prompts for save.
;; We keep it here as a safety belt, in case the back-end forgets
;; to do it.
(dvc-save-some-buffers path)
(dvc-apply "dvc-status" path)))
bzr-dvc.el:
(defalias 'bzr-dvc-status 'bzr-status)
bzr.el:
(defun bzr-status (&optional path)
"Run \"bzr status\"."
(interactive (list default-directory))
(let* ((window-conf (current-window-configuration))
(dir (or path default-directory))
(root (bzr-tree-root dir))
(buffer (dvc-prepare-changes-buffer
`(bzr (last-revision ,root 1))
`(bzr (local-tree ,root))
'status root 'bzr)))
(dvc-switch-to-buffer-maybe buffer)
(dvc-buffer-push-previous-window-config window-conf)
(setq dvc-buffer-refresh-function 'bzr-status)
(dvc-save-some-buffers root)
(dvc-run-dvc-async
'bzr '("status")
:finished
(dvc-capturing-lambda (output error status arguments)
(with-current-buffer (capture buffer)
(if (> (point-max) (point-min))
(dvc-show-changes-buffer output 'bzr-parse-status
(capture buffer))
(dvc-diff-no-changes (capture buffer)
"No changes in %s"
(capture root))))
:error
(dvc-capturing-lambda (output error status arguments)
(dvc-diff-error-in-process (capture buffer)
"Error in diff process"
(capture root)
output error))))))
If we look carefully at the code in bzr.el, there is only one line
that is completely bzr-specific; the spelling of the bzr "status"
command. Everything else can be handled by dispatching functions with
standard names.
So, taking things to the extreme, we _could_ refactor this:
dvc-unified.el:
(defun dvc-status (&optional path)
"Display the status in optional PATH tree."
(interactive)
(let* ((path (when path (expand-file-name path)))
(default-directory (or path default-directory))
(window-conf (current-window-configuration))
(dvc (dvc-current-active-dvc))
(root (dvc-tree-root default-directory))
(buffer (dvc-prepare-changes-buffer
`(,dvc (last-revision ,root 1))
`(,dvc (local-tree ,root))
'status root dvc))))
(dvc-switch-to-buffer-maybe buffer)
(dvc-buffer-push-previous-window-config window-conf)
(setq dvc-buffer-refresh-function (dvc-function dvc "dvc-status"))
(dvc-save-some-buffers root)
(dvc-apply "dvc-status-prep")
(dvc-run-dvc-async
(dvc-apply "dvc-status-cmd")
:finished
(dvc-capturing-lambda (output error status arguments)
(with-current-buffer (capture buffer)
(if (> (point-max) (point-min))
(dvc-show-changes-buffer output (dvc-function "parse-status")
(capture buffer))
(dvc-diff-no-changes (capture buffer)
"No changes in %s"
(capture root))))
:error
(dvc-capturing-lambda (output error status arguments)
(dvc-diff-error-in-process (capture buffer)
"Error in diff process"
(capture root)
output error))))))
bzr-dvc.el:
(defun bzr-status-cmd ()
"status")
Of course, we also need to consider what the other back-ends do. tla
handles nested trees; I'll just ignore that for now (we've agreed DVC
doesn't handle nested trees). The next most different is xmtn, partly
because it fixes some bugs in dvc-run-dvc-async; I'll assume those
fixes are folded in to dvc-run-dvc-async. Then the xmtn back-end for
dvc-status could be:
xmtn-dvc.el:
(defun xmtn-status-prep ()
;; set header, footer messages
;; this implies changing the way dvc-prepare-changes-buffer sets
;; the header and footer
)
(defun xmtn-status-cmd ()
`("automate" "inventory"))
In practice, it's probably more complicated than that; we may not be
able to push the dvc-run-dvc-async macro into the front-end. But the
idea is to push as _much_ common code as possible into the front-end.
dvc-ignore-file-extensions, dvc-ignore-file-extensions-in-dir already
do things in this style.
Doing things this way accomplishes a couple of things:
1) Minimize maintenance complexity by reducing duplicate code among
back-ends.
This is a very important goal for a project with limited manpower.
There is also some current code in dvc-unified that should be
duplicated in the back-ends (handling of interactive argument
defaults); the proposed style eliminates that duplication as well.
2) Maxmize commonality of behavior across back-ends; all back-ends use
the same mode and buffer names for the status buffer, errors are
handled in the same way, etc. The DVC UI is more standardized.
The importance of this goal is less widely accepted.
One downside is that '<back-end>-dvc-status' no longer exists as a
user-callable function. But with the recent introduction of
dvc-back-end-wrappers, I don't see that as a significant downside.
Another downside is that if we run into a new back-end that doesn't
fit the latest factorization, we might have to change things around
again. I think that's a risk worth taking; we have enough back-ends
now to come up with a reasonable factorization.
I think it makes sense to accept this style as the long-term goal for
DVC design, and start moving things in that direction.
Others have objected (or mis-understood) when this issue has come up
in other discussions, but I've never quite understood the objection. I
have usually emphasized goal 2), but I think goal 1) is enough
justification.
For example, I've been working on changing dvc-diff-mode to use
derived types for the ewoc contents (the dvc-fileinfo branch). I've
had to make the same changes (that are _not_ back-end specific) in all
the back-ends; it would be much simpler (and much less error-prone) if
I only had to edit dvc-unified.el instead.
Negotiating exactly what factorization to stop at may be hard, but it
should be fun :).
--
-- Stephe
_______________________________________________
Dvc-dev mailing list
[email protected]
https://mail.gna.org/listinfo/dvc-dev
