Add difftastic-dired-diff

[SqrtMinusOne]

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.

[pkryger]

Thank you for the contribution @SqrtMinusOne!

A couple of points that I'd appreciate your responses:

1. What do you think about naming the function 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).
2. Is there anything that would prevent using 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.
3. Function arguments... These are always fun. I guess something like (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 Allow to override language in magit commands #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.

[SqrtMinusOne]

1. No objections.
2. 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 :-) 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

3. It can be done, I guess. The problem is, as I said, there's a lot going on in the 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.

[pkryger]

1. No objections.

Perfect!

2. 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

3. It can be done, I guess. The problem is, as I said, there's a lot going on in the 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)))

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?

[SqrtMinusOne]

Done 1 and 3, seems to work fine. The issue is #7.

[pkryger]

Would you mind updating documentation to reflect new function name?

[SqrtMinusOne]

Oh, sure.

[pkryger]

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?

[SqrtMinusOne]

Ah, I must have copied that from the describe buffer. Fixed now.