add option to have http links #115
Conversation
| @@ -551,7 +563,8 @@ return (FILENAME . REVISION) otherwise nil." | |||
| (format "L%s" start))))))) | |||
|
|
|||
| (defun git-link-github (hostname dirname filename branch commit start end) | |||
| (format "https://%s/%s/blob/%s/%s" | |||
| (format "%s://%s/%s/blob/%s/%s" | |||
There was a problem hiding this comment.
Hi, thanks for the PR.
When would github ever use an http scheme?
You mentioned in #114 that you only use this for custom gitlab. For that case couldn't you use:
(eval-after-load 'git-link
'(progn
(add-to-list 'git-link-remote-alist
'("mygitlabhost git-link-gitlab))❓
Certainly not opposed to this feature, but if there's not an http use case for the service, why add the code for the world to maintain? And if there is, what is upside to this over git-link-remote-alist addition above (Aside from being terser 😉)?
There was a problem hiding this comment.
Indeed, my implementation is overly-simplistic and handles all hosts in the same way (regardless of the fact that the feature makes little sense when it comes to some of them). Point taken.
To be concrete, your suggestion (as far as I understand it) would look like:
(use-package git-link
:config
(eval-after-load 'git-link
'(progn
(add-to-list
'git-link-remote-alist
'("mygitlabhost"
(lambda (hostname dirname filename branch commit start end)
(format "http://%s/%s/-/blob/%s/%s"
hostname
dirname
(or branch commit)
(concat filename
(when start
(concat "#"
(if end
(format "L%s-%s" start end)
(format "L%s" start))))))))))))Sure, this achieves the desired effect, but I will have to maintain in my configuration a very explicit implementation of something that should be rather an internal detail (and I would have to make sure to keep it in sync as your library evolves). Furthermore, I would have to do a similar thing for git-link-commit-gitlab (quite verbose).
In this respect, my "advice" hack in #114 seems simpler to support (even though it relies on git-link--new).
Let me know if you have another suggestion. If not, I can live with the current state of things (all this seems kind of a detail).
Thanks for the support.
There was a problem hiding this comment.
Yes I see, still have to rewrite function due to hardcoding https. Not nice.
As is, if one wanted http on GitLab they'd also get it for GitHub 
etc... right? It may work but I can see someone opening an issue X months later for this "bug". Any ideas on how to do it per service?
Maybe we could use local repo setting like we do for other things: git-link.scheme
This looks best because you can still have http for local GitLab and https for others.
Maybe we can make scheme last argument to callback and override:
(use-package git-link
:config
(eval-after-load 'git-link
'(progn
(add-to-list
'git-link-remote-alist
'("mygitlabhost"
(lambda (hostname dirname filename branch commit start end) (git-link-gitlab hostname dirname filename branch commit start end "http"))))Still not that clean.
Thoughts?
There was a problem hiding this comment.
(lambda (hostname dirname filename branch commit start end) (git-link-gitlab hostname dirname filename branch commit start end "http"))))
Slightly cleaner with &rest argument to lambda instead.
There was a problem hiding this comment.
I agree with "... opening an issue X months later for this "bug" ..."
Your suggestions goes in the right direction. It boils down to:
(use-package git-link
:config
(eval-after-load 'git-link
(cl-loop
for (alist . function) in '((git-link-remote-alist . git-link-gitlab)
(git-link-commit-remote-alist . git-link-commit-gitlab)
(git-link-homepage-remote-alist . git-link-homepage-github))
do (add-to-list
alist
`("mygitlabhost"
(lambda (&rest args) (apply #',function (append args '("http")))))))))This requires to add &optional scheme to the arguments of git-link-gitlab, git-link-commit-gitlab and git-link-homepage-github and in the format statement to use e.g.,
(format "%s://%s/%s/-/blob/%s/%s"
(or scheme "https")
...Note that I had to use git-link-homepage-github because you reuse it in almost all entries in git-link-homepage-remote-alist so probably we have to define a dedicated function git-link-homepage-gitlab and I think that it would be good to rename git-link-homepage-github to something like git-link-homepage-common.
I am not sure how using git-link.scheme would help. For me, having to add properties repository by repository to get a features like this is inconvenient.
Having said all this, as a user, I would expect to have to do something like:
(use-package git-link
:config
(git-link-register-gitlab-host "mygitlabhost" "http"))where the library itself provides something like:
(defun git-link-register-gitlab-host (host-name &optional scheme)
(eval-after-load 'git-link
(cl-loop
for (alist . function) in '((git-link-remote-alist . git-link-gitlab)
(git-link-commit-remote-alist . git-link-commit-gitlab)
(git-link-homepage-remote-alist . git-link-homepage-github))
do (add-to-list
alist
`(,host-name
(lambda (&rest args) (apply #',function (append args (when ,scheme '(,scheme)))))))))) but as an interface this looks strange (we handle one case - gitlab).
There was a problem hiding this comment.
I am not sure how using git-link.scheme would help. For me, having to add properties repository by repository to get a features like this is inconvenient.
We want to avoid setting git-link-http-link for custom GitLab, causing http to get used for normal GitLab, GitHub, etc... This is a per-repo setting. git-link.scheme is per-repo.
On the elisp side, what's the repo/project version of a buffer-local variable? I don't think they exist.
Having said all this, as a user, I would expect to have to do something like:
(use-package git-link
:config
(git-link-register-gitlab-host "mygitlabhost" "http"))
Is this any more inconvenient than git-link.scheme? This package is 10 years old and this is the first time someone has needed it (at least publicly). http is the exception case. How often will one truly be inconvenienced by this?
There was a problem hiding this comment.
On the elisp side, what's the repo/project version of a buffer-local variable? I don't think they exist.
What do you mean?
We want to avoid setting git-link-http-link for custom GitLab, causing http to get used for normal GitLab, GitHub, etc... This is a per-repo setting. git-link.scheme is per-repo.
I don't insist about any of this - I just share my opinion. For me the most convenient way to resolve this is to define "properties" per host (and not per project). Doing something per project wouldn't fit my workflow (but this is only me, I guess).
This package is 10 years old and this is the first time someone has needed it
I can live with my own hack (there is no problem).
There was a problem hiding this comment.
On the elisp side, what's the repo/project version of a buffer-local variable? I don't think they exist.
What do you mean?
The http/https setting is per project, right? In the current PR setting git-link-http-link will set it globally. As far as I know there's no way to do this via Emacs Lisp using a variable. There's buffer-local which kinda works but buffer scope is way too narrow.
I don't insist about any of this - I just share my opinion. For me the most convenient way to resolve this is to define "properties" per host (and not per project). Doing something per project wouldn't fit my workflow (but this is only me, I guess).
To me it seems very simple for current PR to replace git-link-http-link check with git-link.scheme git config check. The property approach is broader than that, and may even be functionality on top of this. Let me take a look at your config a bit more to make sure I understand.
Related to #114.