Skip to content
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

Smartly guess the protocol and port for the remote provider #1984

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Apr 28, 2022

Description

Currently the remote provider uses https by default and allow users to set a different protocol if they want in the configuration.

This is kept, but now, we try to infer the best protocol by looking at the git remote URL.

And, previously, no custom ports were allowed to be set. This is still true, but now, we try to infer the best port by looking at the git remote URL.

Fixes #632
Fixes #1918
Refs #1964

Maybe more.

TODOs

  • Refactor the default value of the setting remoteProvider.protocol from https to auto

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

@felipecrs
Copy link
Contributor Author

@eamodio, @oliverdunk this is a draft for what we have been discussing in #1964. It would be a requirement for implementing support for urls.base (if we go by the approach I mentioned).

Do not worry about reviewing this in details (I expect things to be missing, which I will caught yet), but I would like to have your +1 in the route I'm taking with this PR before spending more time on it.

Any comments are appreciated.

@oliverdunk
Copy link

Hey @felipecrs, thanks for poking at this! Two things that crossed my mind looking at the diff:

  • Introducing a new library to parse URLs and new data structure to store them in feels like quite a large change. I'm fine with that if everyone agrees it's the best approach, but if we want to keep the changes minimal I feel like there are other ways to get the protocol using the existing code.
  • Is changing the constructor of these providers a breaking change and is that a concern? Presumably other extensions that integrate with GitLens could be instantiating them.

I've never contributed to GitLens so am just replying here since I'm keen to see something land! All of this should be taken with a grain of salt.

@felipecrs
Copy link
Contributor Author

Very good points!

@kumekay
Copy link

kumekay commented Jul 25, 2022

@felipecrs Do you have any plans to continue work on this PR?

@felipecrs
Copy link
Contributor Author

@felipecrs Do you have any plans to continue work on this PR?

Sorry, no. Feel free to take it over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants