Create Issue button #8

jens1o · 2017-06-30T19:36:21Z

fixes #7

This method is quite slow, I suggest we invest somewhat in caching. But this is out of scope in this pr.

Maybe there is another icon we could use which fits better, but I focused on the code.

msftclas · 2017-06-30T19:36:25Z

@jens1o,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

chrmarti · 2017-06-30T20:21:01Z

resources/general/createIssue.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"><polygon points="13,2 8,2 6,2 6,0 2,0 2,2 0,2 0,6 2,6 2,8 4,8 4,16 16,16 16,5" fill="#F6F6F6"/><polygon points="12,3 8,3 8,4 11,4 11,7 14,7 14,14 6,14 6,8 5,8 5,15 15,15 15,6" fill="#424242"/><path d="M7 3.018h-2v-2.018h-1.981v2.018h-2.019v1.982h2.019v2h1.981v-2h2v-1.982z" fill="#388A34"/><polygon points="11,7 11,4 8,4 8,6 6,6 6,8 6,14 14,14 14,7" fill="#F0EFF1"/></svg>


Where is the icon from? (I'm asking because we need to make sure it has an appropriate license.)

It's from here: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/files/browser/media/AddFile.svg

chrmarti · 2017-06-30T20:27:49Z

src/github-issues-prs.ts

+		}
+
+		// take first one
+		await open(getGitHubUrl(remotes[0].url) + '/issues/new');


The first one might not be the right one. Maybe you could let the user choose one with window.showQuickPick() when there are multiple remotes?

Sounds reasonable, but should we remember it, and if yes, where?

I wouldn't remember it for now to keep things simple.

chrmarti · 2017-06-30T20:35:09Z

src/github-issues-prs.ts

+ * Helper function which removes the ending `.git`
+ */
+function getGitHubUrl(url: string): string {
+	return url.substr(0, url.lastIndexOf('.git'));


The URL might be in SSH format (e.g., git@github.com:octocat/Hello-World.git). You can instead use github.repos.get(...) (see https://developer.github.com/v3/repos/#get) to get the repository's data and use html_url of that.

Ah, didn't played with the js api tbh, thanks!

jens1o · 2017-07-01T09:14:13Z

like this?

chrmarti · 2017-07-03T17:48:15Z

src/github-issues-prs.ts

@@ -104,10 +104,37 @@ export class GitHubIssuesPrsProvider implements TreeDataProvider<TreeItem> {
 			return false;
 		}

-		// take first one
-		await open(getGitHubUrl(remotes[0].url) + '/issues/new');
+		let urls: string[] = remotes.map(remote => remote.url);


Check if there is no url and show an info message in that case.

Also check if there is only one remote and use that directly.

chrmarti · 2017-07-03T17:49:16Z

src/github-issues-prs.ts


-		return true;
+		window.showQuickPick(


Use async/await: const remote = await window.showQuickPick(...);

chrmarti · 2017-07-03T17:49:43Z

src/github-issues-prs.ts


-		return true;
+		window.showQuickPick(
+			Promise.resolve(urls),


No need to wrap in a promise, use just 'urls'.

chrmarti · 2017-07-03T17:55:16Z

src/github-issues-prs.ts

+
+
+			// get my object back
+			const selectedRemote = remotes.find(x => x.url === remote);


Actually, instead of passing 'urls' above you could pass an array of QuickPickItem with a label that is 'owner/repo' and an additional property 'remote'. That will give a nicer label (and more flexibility). You'll get the selected QuickPickItem as the result and can directly use the remote from that.

chrmarti · 2017-07-03T17:57:07Z

src/github-issues-prs.ts

+
+			const github = new GitHub();
+
+			github.repos.get({


Use async/await: const data = await github.repos.get(...);

It does not support that, I'm getting the error that the method does not support an await statement, because it returns a Promise and is not an asynchronous function.

chrmarti · 2017-07-03T17:59:51Z

src/github-issues-prs.ts

+				// TODO: Store in cache
+				open(data.data.html_url + '/issues/new')
+			}).catch(() => {
+				window.showInformationMessage('Failed fetching data from the GitHub api!');


That discards the error, maybe using 'await' on 'github.repos.get(...)' above is enough for now, that will let the error propagate up and an error message will be shown.

chrmarti · 2017-07-03T18:01:38Z

Yes, I have added a few more comments. Please take a look.

jens1o · 2017-07-04T03:57:38Z

Okay, I think I can handle them later this day. Thanks for taking a look. :)

jens1o · 2017-07-05T18:47:13Z

I resolved most of the problems, sorry for the delay, end of the school year with lots of exams.

chrmarti · 2017-07-10T16:41:01Z

src/github-issues-prs.ts

+			if (!selectedRemote) {
+				return;
+			}
+			console.log(selectedRemote.remote);


Remove console.log.

chrmarti · 2017-07-10T16:45:02Z

src/github-issues-prs.ts

+			console.log(selectedRemote.remote);
+
+			const github = new GitHub();
+


Add authentication to support private repositories (doesn't work in all cases, but we can improve that later):

if (remote.username && remote.password) { github.authenticate({ type: 'basic', username: remote.username, password: remote.password }); }

Good idea! :)

chrmarti · 2017-07-10T16:46:38Z

@jens1o No problem, I was busy last week too. Just added a few more comments. Looks good otherwise!

jens1o · 2017-07-11T17:12:43Z

Thanks for merging :)

chrmarti · 2017-07-11T18:04:56Z

Thanks for submitting! Curiously there were a few TypeScript errors, you might want to check you are using the latest version of TypeScript. I have fixed these.

I have replaced the somewhat heavy 'New File' icon with a simpler '+', hope that's fine with you.

jens1o · 2017-07-11T18:19:37Z

Yep, that's fine.

Oh, I didn't noticed that somehow... Thanks for the heads up!

jens1o added 2 commits June 30, 2017 20:55

🚀 Support creating issues

9cd899a

finish up pr

70c3f31

msftclas added the cla-already-signed label Jun 30, 2017

trim trailing whitespaces

d8a7d4c

chrmarti suggested changes Jun 30, 2017

View reviewed changes

implement picker

cce6a57

chrmarti self-assigned this Jul 3, 2017

chrmarti reviewed Jul 3, 2017

View reviewed changes

cleanup

39e8696

remove debug helper

d8ea44f

chrmarti reviewed Jul 10, 2017

View reviewed changes

support private repos and cleanup

5e526ee

chrmarti merged commit 9f1f8a8 into microsoft:master Jul 11, 2017

jens1o deleted the jens1o-create-issue branch July 11, 2017 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Issue button #8

Create Issue button #8

jens1o commented Jun 30, 2017 •

edited

Loading

msftclas commented Jun 30, 2017

chrmarti Jun 30, 2017

jens1o Jun 30, 2017

chrmarti Jun 30, 2017

jens1o Jun 30, 2017

chrmarti Jun 30, 2017

jens1o Jun 30, 2017

chrmarti Jun 30, 2017

jens1o Jun 30, 2017

jens1o commented Jul 1, 2017

chrmarti Jul 3, 2017

chrmarti Jul 3, 2017

chrmarti Jul 3, 2017

chrmarti Jul 3, 2017

chrmarti Jul 3, 2017

chrmarti Jul 3, 2017

jens1o Jul 5, 2017

chrmarti Jul 3, 2017

chrmarti commented Jul 3, 2017

jens1o commented Jul 4, 2017

jens1o commented Jul 5, 2017

chrmarti Jul 10, 2017

jens1o Jul 10, 2017

chrmarti Jul 10, 2017

jens1o Jul 10, 2017

chrmarti commented Jul 10, 2017

jens1o commented Jul 11, 2017

chrmarti commented Jul 11, 2017

jens1o commented Jul 11, 2017

		@@ -0,0 +1 @@
		<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"><polygon points="13,2 8,2 6,2 6,0 2,0 2,2 0,2 0,6 2,6 2,8 4,8 4,16 16,16 16,5" fill="#F6F6F6"/><polygon points="12,3 8,3 8,4 11,4 11,7 14,7 14,14 6,14 6,8 5,8 5,15 15,15 15,6" fill="#424242"/><path d="M7 3.018h-2v-2.018h-1.981v2.018h-2.019v1.982h2.019v2h1.981v-2h2v-1.982z" fill="#388A34"/><polygon points="11,7 11,4 8,4 8,6 6,6 6,8 6,14 14,14 14,7" fill="#F0EFF1"/></svg>



		// get my object back
		const selectedRemote = remotes.find(x => x.url === remote);

		console.log(selectedRemote.remote);

		const github = new GitHub();

Create Issue button #8

Create Issue button #8

Conversation

jens1o commented Jun 30, 2017 • edited Loading

msftclas commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jens1o commented Jul 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrmarti commented Jul 3, 2017

jens1o commented Jul 4, 2017

jens1o commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrmarti commented Jul 10, 2017

jens1o commented Jul 11, 2017

chrmarti commented Jul 11, 2017

jens1o commented Jul 11, 2017

jens1o commented Jun 30, 2017 •

edited

Loading