-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add option to have http links #115
Open
drdv
wants to merge
1
commit into
sshaw:master
Choose a base branch
from
drdv:feature/http-links
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
❓
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 overgit-link-remote-alist
addition above (Aside from being terser 😉)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 andhttps
for others.Maybe we can make scheme last argument to callback and override:
Still not that clean.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly cleaner with
&rest
argument to lambda instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with "... opening an issue X months later for this "bug" ..."
Your suggestions goes in the right direction. It boils down to:
This requires to add
&optional scheme
to the arguments ofgit-link-gitlab
,git-link-commit-gitlab
andgit-link-homepage-github
and in theformat
statement to use e.g.,Note that I had to use
git-link-homepage-github
because you reuse it in almost all entries ingit-link-homepage-remote-alist
so probably we have to define a dedicated functiongit-link-homepage-gitlab
and I think that it would be good to renamegit-link-homepage-github
to something likegit-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:
where the library itself provides something like:
but as an interface this looks strange (we handle one case -
gitlab
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
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).
I can live with my own hack (there is no problem).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.To me it seems very simple for current PR to replace
git-link-http-link
check withgit-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.