Closed mijoharas closed 5 months ago
Hi thanks. Yes a longstanding and sadly ongoing issue with this module.
I think issue with global solution is that the function call should be "local" to the code in which it's being called and not needlessly depend on global behavior. You can overcome this with let
call so maybe that is sufficient? It is certainly is some Elisp type antics.
I think this is the problem with other solutions that have been submitted: https://github.com/sshaw/git-link/issues/118#issuecomment-1975204223 What do you think?
Though after all this time not having this functionality, maybe it is a fine trade-off.
Oh and 1 important point: other code should not be calling git-link--new
directly this is a "private" function per conventions. A new function should be make available for code-only calling.
Hi, so, I'm not directly calling git-link--new
in what I'm trying to do, but since the return value of git-link--new
is the return value of all of the public functions, I would need to modify that to modify the public functions. Before this change I had hacked in a cl-letf
to locally override the git-link--new
function because I didn't want what it did. With this change I can get rid of that hack.
Note: I am calling this interactively for my usecase. (and using a local let
binding for it with the new code). Could you expand on the problems with calling it interactively like this that you mention in https://github.com/sshaw/git-link/issues/118#issuecomment-1975204223?
Let me show you one of the functions that I was writing:
Before this change (hacky):
(defun mike/magit-go-to-github-commit-hash-copied ()
"Go to the github commit page at the hash under the chunk of the magit blame"
(interactive)
(let* ((hash (oref (magit-current-blame-chunk) orig-rev)) ;; elpa/28.2/develop/magit-20230523.1431/magit-blame.el:920:28
(base_url
(cl-letf (((symbol-function 'git-link--new) (lambda (x) x)))
(call-interactively 'git-link-homepage)))
(full_url (concat base_url "/commits/" hash)))
(message full_url)
(browse-url full_url)
)
)
After this change (imo, cleaner):
(defun mike/magit-go-to-github-commit-hash-copied ()
"Go to the github commit page at the hash under the chunk of the magit blame"
(interactive)
(require 'git-link)
(let* ((hash (oref (magit-current-blame-chunk) orig-rev))
(git-link-open-in-browser nil)
(git-link-add-to-kill-ring nil)
(base_url
(call-interactively 'git-link-homepage))
(full_url (concat base_url "/commits/" hash)))
(message full_url)
(browse-url full_url)
)
)
Note: I don't see myself globally setting that flag, I only really want it for local modification, but it seemed like the cleanest way to do what I wanted, and I figured someone might want to configure it.
Note 2: I think I may have tried calling the function explicitly with remote
/start
/end
instead of calling it interactively, but then found I was just duplicating some of the logic of the function, so figured call-interactively
was better for my usecase but :man_shrugging: .
Thanks for the explanation and sorry for the delay. Makes sense.
I think there is also a feature request for linking to the blame for the given line. I should probably look into that as certainly others are doing what you're doing
The business about call-interactively
, I can't remember the issues maybe with argument handling? There was a write-up on Emacs Wiki(?) about it but it's no longer there so maybe no longer an issue. 🤷
Thanks! (For what it's worth, in the meantime I reread the other issues and do think a "pure" function would probably be good in general, but for what I want the call-interactively is better to not duplicate the extra code in the interactive block, I'll report back if I ever run into troubles using it though).
Thanks again!
Additionally return the string from the
git-link--new
function. This makes it simpler to automatically call git-link functions from other code.