Support host name(fix #25) by Ikuyadeu · Pull Request #27 · microsoft/vscode-github-issues-prs

Ikuyadeu · 2017-10-19T07:43:24Z

It's one idea to fix #25.
And, If you use GitHubAPI, this change has no effect.

chrmarti · 2017-10-19T14:43:10Z

package.json

          "default": [],
          "description": "A list of repositories to query for issues."
+        },
+        "github.host": {


Would it be difficult to support multiple hosts?

I think, if you set multiple hosts, you need to set multiple user names and passwords for each hosts.

Sounds good.

chrmarti · 2017-10-19T14:46:24Z

src/github-issues-prs.ts

 	private children: Promise<TreeItem[]> | undefined;

 	private username: string | undefined;
+	private host: string | undefined;


getGitHubRemotes() has two references to "github.com". I guess that needs to be updated too. Do you have access to a GitHub Enterprise install?

Thanks, I'll fix.
But, I don't have GHE. How should I test for GHE environment?

chrmarti · 2017-11-03T20:29:36Z

src/github-issues-prs.ts

 				for (const url of new Set(allMatches(/^[^\s]+\s+([^\s]+)/gm, stdout, 1))) {
-					const m = /[^\s]*github\.com[/:]([^/]+)\/([^ ]+)[^\s]*/.exec(url);
+					// const m = /[^\s]*github\.com[/:]([^/]+)\/([^ ]+)[^\s]*/.exec(url);
+					const m = new RegExp(`[^\s]*${this.getURL()}[/:]([^/]+)\/([^ ]+)[^\s]*`).exec(url);


Backslashes need escaping in the string. Slashes don't. The URL might need some escaping too. Maybe (please check):

const m = new RegExp(`[^\\s]*${this.getURL().replace(/\./g, '\\.')}[/:]([^/]+)/([^ ]+)[^\\s]*`).exec(url);

chrmarti · 2017-11-03T20:30:38Z

src/github-issues-prs.ts

+	}
+
+	private getURL() {
+		if(this.host === undefined){


Shouldn't that always be defined?

chrmarti · 2017-11-03T20:34:01Z

src/github-issues-prs.ts

 				let remote = remotes[`${owner}/${repo}`];
 				if (!remote) {
-					const url = `https://github.com/${owner}/${repo}.git`;
+					const url = `${this.host}/${owner}/${repo}.git`;


Should that keep the https?

Ikuyadeu · 2017-11-04T08:53:27Z

@chrmarti Thank you for review.
How is this?

chrmarti

Thanks for the update. I've added a few more comments.

chrmarti · 2017-11-06T18:00:39Z

src/github-issues-prs.ts

 			const item = issue.item;
 			const assignee = issue.query.assignee;
-			const url = `https://github.com/${issue.query.remote.owner}/${issue.query.remote.repo}/issues?q=is%3Aopen+milestone%3A%22${item.milestone.title}%22${assignee ? '+assignee%3A' + assignee : ''}`;
+			const url = `https://${this.getURL}/${issue.query.remote.owner}/${issue.query.remote.repo}/issues?q=is%3Aopen+milestone%3A%22${item.milestone.title}%22${assignee ? '+assignee%3A' + assignee : ''}`;


getURL is missing ().

chrmarti · 2017-11-06T18:03:05Z

src/github-issues-prs.ts

 	}
+
+	private getAPIOption() {
+		if (this.host === "github.com") {


Could you use single quotes ' instead of double quotes? Just for consistency with the rest of the code.

chrmarti · 2017-11-06T18:06:21Z

src/github-issues-prs.ts

 				const { stdout } = await exec('git remote -v', { cwd: folder.uri.fsPath });
 				for (const url of new Set(allMatches(/^[^\s]+\s+([^\s]+)/gm, stdout, 1))) {
-					const m = /[^\s]*github\.com[/:]([^/]+)\/([^ ]+)[^\s]*/.exec(url);
+					const m = new RegExp(`[^\s]*${this.getURL().replace(/\./g, '\\.')}[/:]([^/]+)\/([^ ]+)[^\s]*`).exec(url);


I think the \s still need escaping as \\s, don't they? That's because it is in a string and not in a regex anymore.

I missed it. Thank you

chrmarti · 2017-11-06T18:14:12Z

src/github-issues-prs.ts

 				let remote = remotes[`${owner}/${repo}`];
 				if (!remote) {
-					const url = `https://github.com/${owner}/${repo}.git`;
+					const url = `https://${this.host}/${owner}/${repo}.git`;


Does this not need the encoding of getURL()? Do you have an example of when the encoding is needed?

e.g. Thyme &time=again to comment=Thyme%20&time=again.
However, I think host name doesn't have space.
I'll remove this. Thank you.

chrmarti · 2017-11-06T18:14:37Z

package.json

          "default": [],
          "description": "A list of repositories to query for issues."
+        },
+        "github.host": {


Sounds good.

mikehaas763 · 2017-11-07T19:43:06Z

I haven't done any vs code extension development before, what's the best and/or easiest way for me to test this out? Do we pass around vsix files or something?

chrmarti · 2017-11-07T23:14:27Z

@mikehaas763 You could checkout this PR from @Ikuyadeu's repo and open it in VS Code. Then run npm install in its folder and launch a second VS Code window with the extension running by pressing F5 in the first window.

chrmarti · 2017-11-08T22:46:14Z

Thanks @Ikuyadeu LGTM! I'll merge this even before it is tested so we don't run into merge conflicts with other changes.

chrmarti · 2018-01-18T11:24:33Z

@Ikuyadeu I missed giving you credit in the 1.19 release notes due to being on vacation. Sorry about that. I will do so in the 1.20 release notes. Thanks again!

Ikuyadeu · 2018-01-18T11:50:53Z

@chrmarti OK! Thank you too!

Support host name(fix microsoft#25)

7d11e9c

vscodebot bot assigned chrmarti Oct 19, 2017

chrmarti reviewed Oct 19, 2017

View reviewed changes

chrmarti added this to the October 2017 milestone Oct 19, 2017

chrmarti removed this from the October 2017 milestone Oct 30, 2017

chrmarti mentioned this pull request Nov 2, 2017

discussion: what would it take to make this work with enterprise github? #25

Closed

fix for some references

e76d66b

chrmarti reviewed Nov 3, 2017

View reviewed changes

Ikuyadeu added 2 commits November 4, 2017 17:50

Fix url miss and type of this.host

2935e07

remove extra define

5621ffd

chrmarti added this to the October 2017 milestone Nov 6, 2017

chrmarti reviewed Nov 6, 2017

View reviewed changes

Fix based on chrmarti review

d286a4e

chrmarti modified the milestones: October 2017, November 2017 Nov 8, 2017

chrmarti merged commit 014b16a into microsoft:master Nov 8, 2017

Ikuyadeu deleted the GHE branch November 9, 2017 00:35

chrmarti modified the milestones: November 2017, January 2018 Jan 18, 2018

Conversation

Ikuyadeu commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ikuyadeu commented Nov 4, 2017

Uh oh!

chrmarti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikehaas763 commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrmarti commented Nov 7, 2017

Uh oh!

chrmarti commented Nov 8, 2017

Uh oh!

chrmarti commented Jan 18, 2018

Uh oh!

Ikuyadeu commented Jan 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mikehaas763 commented Nov 7, 2017 •

edited

Loading