Open Repository in ext browser with linked branch #280
Conversation
| private static final String GITHUB_DOMAIN = "github.com"; //$NON-NLS-1$ | ||
| private static final String GITLAB_DOMAIN = "gitlab.com"; //$NON-NLS-1$ | ||
| private static final String GIT_WDF_SAP_DOMAIN = "github.wdf.sap.corp"; //$NON-NLS-1$ | ||
| private static final String GITHUB_INFRA_DOMAIN = "github.infra.hana.ondemand.com"; //$NON-NLS-1$ |
There was a problem hiding this comment.
Let's rename GITHUB_INFRA_DOMAIN to GITHUB_INFRA_HANA_DOMAIN
There was a problem hiding this comment.
Yes, changed all occurrences
| // Get Repository link | ||
| String repoLink = getLink(repository); | ||
| // Open the link in default external browser | ||
| URI uri = new URI(repoLink); |
There was a problem hiding this comment.
Do we need to first create a URI using the link & then fetch the URL ?
Any specific reason?
Is creating a new URL object with the link not sufficient?
There was a problem hiding this comment.
The URL() function is deprecated and when I was going through declaration I found it suggests to use URI.toURL()
| @@ -22,6 +23,12 @@ | |||
| */ | |||
| public class OpenRepositoryAction extends Action { | |||
| private final IViewPart view; | |||
| private static final String GITHUB_DOMAIN = "github.com"; //$NON-NLS-1$ | |||
| private static final String GITLAB_DOMAIN = "gitlab.com"; //$NON-NLS-1$ | |||
| private static final String GIT_WDF_SAP_DOMAIN = "github.wdf.sap.corp"; //$NON-NLS-1$ | |||
There was a problem hiding this comment.
Let's rename GIT_WDF_SAP_DOMAIN to GITHUB_WDF_SAP_DOMAIN
There was a problem hiding this comment.
Yes, changed all occurrences
| } catch (URISyntaxException e) { | ||
| // TODO Auto-generated catch block | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
- Exceptions should be logged and if required should also provide feedback to users of the error occured
Add the exception to existing clause, where the error message is shown to users in a MessageDialog & the exception is also logged in the plugin.
There was a problem hiding this comment.
Yes, took reference from existing code
| // Extract domain by splitting with '/' | ||
| domain = domain.split("/")[0]; //$NON-NLS-1$ | ||
| // Get connected branch | ||
| String branch = repository.getBranchName(); |
There was a problem hiding this comment.
Move line 76 after line 68 & remove comment for better readability.
| // Remove protocol (http:// or https://) | ||
| String domain = repoLink.replaceFirst("https?://", ""); //$NON-NLS-1$ //$NON-NLS-2$ | ||
|
|
||
| // Extract domain by splitting with '/' | ||
| domain = domain.split("/")[0]; //$NON-NLS-1$ |
There was a problem hiding this comment.
Check if you can create a URI using the repoLink & then getHost from the URI as domain.
There was a problem hiding this comment.
Changed string based change to URI class to handle url generation
| // handle domain based repoLink generation | ||
| if (domain.equals(GITHUB_DOMAIN) || domain.equals(GITLAB_DOMAIN) || domain.equals(GITHUB_TOOLS_DOMAIN) | ||
| || domain.equals(GITHUB_INFRA_DOMAIN) || domain.equals(GIT_WDF_SAP_DOMAIN)) { | ||
| branch = branch.replace("refs/heads/", "/tree/"); //$NON-NLS-1$//$NON-NLS-2$ |
There was a problem hiding this comment.
Extract refs/heads/ as a constant
| || domain.equals(GITHUB_INFRA_DOMAIN) || domain.equals(GIT_WDF_SAP_DOMAIN)) { | ||
| branch = branch.replace("refs/heads/", "/tree/"); //$NON-NLS-1$//$NON-NLS-2$ | ||
| } else if (domain.endsWith(BIT_BUCKET_DOMAIN)) { | ||
| repoLink = repoLink.replaceAll("https://.*?@", "https://"); //$NON-NLS-1$ //$NON-NLS-2$ |
There was a problem hiding this comment.
You can also remove the username in BitBucket links by using URI to parse the repoLink.
Then rebuild the URI without userInfo & get URL.
Check constructor for URI:
public URI(String scheme,
String userInfo, String host, int port,
String path, String query, String fragment)
There was a problem hiding this comment.
Also check if other GIT providers can have user info.
In such case, we could have a domain agnostic user info handler.
There was a problem hiding this comment.
Didn't get any user info in case of git providers.
| } | ||
|
|
||
| // return the concatenated link that redirects to the branch | ||
| return repoLink + branch; |
There was a problem hiding this comment.
Handle the case when the domain is not the list that is supported for navigation to branch.
Presently, branch name is added to such domains. Instead unsupported domains should just have repository link for navigation without branch name.
There was a problem hiding this comment.
default repoLink is the url, now only if we have supported domain then we will add branch else will be simply redirected to the repo.
Workflow update:
Earlier:
Now