Closed SqrtMinusOne closed 7 months ago
Thank you for the contribution @SqrtMinusOne!
A couple of points that I'd appreciate your responses:
difftastic-dired-diff
? To my mind it will make a clear point what this function does just looking at the name (or at least the hint will be more pronounced).difftastic--with-temp-advice
with :override
instead of cl-letf
? I guess that would make it common with how the rest of the package interjects with other functions.(FILE &optional LANG-OVERRIDE)
would make sense here. That way it will be callable similarly to dired-diff
, yet having the similar call semantic to other difftastic-
interactive functions (I am planning to add it for magit
commands, see #4). The LANG-OVERRIDE
will be used instead of dired-diff
's SWITCHES
, so perhaps clearing prefix before calling dired-diff
should be added. Perhaps you have a better idea. If you agree this should be updated, but you're not sure if you can work on this immediately, perhaps just create an issue for future extension. Let me know what your thoughts are.
...advice should be reserved for the cases where you cannot modify a function’s behavior in any other way
I'd rather implement that part with cl-letf
:-) E.g.:
(defun my/test (a b)
(+ a b))
(let ((original-fun (symbol-function 'my/test)))
(cl-letf (((symbol-function 'my/test)
(lambda (a b)
(+ (funcall original-fun a b)
1))))
(my/test 2 2)))
=> 5
interactive
block of dired-diff
, and I don't think there's any way to extract the results of that without actually calling the function. Something like this would technically work:
(defun difftastic-dired-diff (file &optional lang-override)
"Compare file at point with FILE using difftastic.
The behavior is the same as `dired-diff'." (interactive (list 'interactive (when current-prefix-arg (completing-read "Language: " (difftastic--languages) nil t)))) (cl-letf (((symbol-function 'diff) (lambda (current file _switches) (difftastic-files current file lang-override))) (current-prefix-arg nil)) (if (eq file 'interactive) (call-interactively #'dired-diff)) (funcall #'dired-diff file)))
However, I see there are a lot of other options in `difft`. Maybe a transient prefix would make more sense, instead of or in addition to the above.
At this point it would have to modify the global `process-environment`, otherwise it would take a lot of refactoring to pass the options though every function... I can try doing something like that.
- No objections.
Perfect!
- Ah, I didn't notice that macro. I don't think "temporary advising" is the right approach though, for what it's worth, the manual says:
...advice should be reserved for the cases where you cannot modify a function’s behavior in any other way
I'd rather implement that part with
cl-letf
:-) [...]
Let's stay with cl-letf
- It can be done, I guess. The problem is, as I said, there's a lot going on in the
interactive
block ofdired-diff
, and I don't think there's any way to extract the results of that without actually calling the function. Something like this would technically work:(defun difftastic-dired-diff (file &optional lang-override) "Compare file at point with FILE using difftastic. The behavior is the same as `dired-diff'." (interactive (list 'interactive (when current-prefix-arg (completing-read "Language: " (difftastic--languages) nil t)))) (cl-letf (((symbol-function 'diff) (lambda (current file _switches) (difftastic-files current file lang-override))) (current-prefix-arg nil)) (if (eq file 'interactive) (call-interactively #'dired-diff)) (funcall #'dired-diff file)))
I like this approach. Would you mind checking it and updating the PR?
However, I see there are a lot of other options in
difft
. Maybe a transient prefix would make more sense, instead of or in addition to the above.At this point it would have to modify the global
process-environment
, otherwise it would take a lot of refactoring to pass the options though every function... I can try doing something like that.
This is a really good point. I'd suggest that for now let's stay with just lang-override
. I found it quite useful in a few places so far. But a proper transient
would be a nice thing to have. Would you mind opening an issue where we could discuss it further?
Done 1 and 3, seems to work fine. The issue is #7.
Would you mind updating documentation to reflect new function name?
Oh, sure.
Sorry to bother but linter is bot happy ☹️:
difftastic.el:1048:2: Error: docstring has wrong usage of unescaped single quotes (use \=' or different quoting such as `...')
AFK now. Would you mind taking care of this?
Ah, I must have copied that from the describe buffer. Fixed now.
This adds a version of
dired-diff
that makes use of this package.There's a lot going on in the interactive block of the original function, so I thought
cl-letf
would make more sense than just copying the function.