Show workflow jobs and steps #152

alexweininger · 2020-07-20T18:02:47Z

Not sure how Nathan's commits got on this PR, but if that's an issue I will try to fix it.

A lot of added code here is added to deal with displaying the times and date completed of the jobs and steps. We might try to rely on an npm package to do this for us, however I didn't immediately find one that suited our needs.

I also feel that I need to write tests for the util functions I added. I will write these soon I just wanted to get eyes on the code asap.

src/utils/timeUtils.ts

src/tree/ActionTreeItem.ts

src/constants.ts

src/tree/JobTreeItem.ts

src/utils/gitHubUtils.ts

nturinski · 2020-07-21T00:34:12Z

src/utils/timeUtils.ts

+ * **NOTE:** If the difference between `start` and `end` is negative, `end` will be treated as the current time.
+ */
+// tslint:disable-next-line: export-name
+export function getTimeElapsedString(start: Date, end: Date): string {


I think you can probably replace this with this https://momentjs.com/docs/#/displaying/difference/

I can't see any way to cleanly format a moment object to 2m 31s.

It seems like it's a missing feature that people have wanted for a while and is still not implemented in moment: moment/moment#463

I've looked at both

https://momentjs.com/docs/#/displaying/format/

and https://momentjs.com/docs/#/durations/humanize/

And neither can do the job.

IMO it feels like you're trying to pack too much info into the description. This piece feels the least important relative to it's code complexity and I would get rid of it.

src/tree/ActionTreeItem.ts

src/constants.ts

ejizba · 2020-07-21T17:32:32Z

src/utils/timeUtils.ts

+ * **NOTE:** If the difference between `start` and `end` is negative, `end` will be treated as the current time.
+ */
+// tslint:disable-next-line: export-name
+export function getTimeElapsedString(start: Date, end: Date): string {


IMO it feels like you're trying to pack too much info into the description. This piece feels the least important relative to it's code complexity and I would get rid of it.

src/utils/gitHubUtils.ts

ejizba · 2020-07-21T17:38:25Z

src/constants.ts

+
+// Doc for these parameter values: https://developer.github.com/v3/checks/runs/#parameters
+
+export enum Conclusion {


These types should stay in the same file as the type for GitHubAction since they're so closely related. If you don't like having it in ActionTreeItem, make a separate file for GitHub stuff. The separate file would somewhat represent a drop-in replacement for the GitHub npm package that I assume you guys will switch to soon

I think you missed this comment-- we probably could move this to the gitHubTypings. Annoying that they don't have their own enum for this, but alas

Oops! Will do.

src/tree/StepTreeItem.ts

src/constants.ts

…ticwebapps into alex/workflowJobs

src/tree/ActionsTreeItem.ts

nturinski · 2020-07-24T01:10:21Z

src/tree/JobTreeItem.ts

+    public get description(): string {
+        if (this.data.conclusion !== null) {
+            const elapsedMs: number = this.completedDate.getTime() - this.startedDate.getTime();
+            return `${convertConclusionToVerb(<Conclusion>this.data.conclusion)} ${moment(this.completedDate).fromNow()} in ${prettyMs(elapsedMs)}`;


So I wanted to confirm with GitHub, but apparently it doesn't support any language other than English: https://github.blog/2010-07-13-github-in-your-language/

~~I feel like we might want to localize this string because I'm not sure this format is standard in say, Chinese or something.~~

Actually, moment is writing complete phrases like 21 days ago or 21 minutes ago, so I'm not sure how we would localize that unless moment has language support.

nturinski · 2020-07-24T01:13:05Z

src/tree/JobTreeItem.ts

+    public get description(): string {
+        if (this.data.conclusion !== null) {
+            const elapsedMs: number = this.completedDate.getTime() - this.startedDate.getTime();
+            return `${convertConclusionToVerb(<Conclusion>this.data.conclusion)} ${moment(this.completedDate).fromNow()} in ${prettyMs(elapsedMs)}`;


Rather than casting this.data.conclusion, you could make a get property for conclusion that confirms it's one of the enums and then returns it casted.

https://stackoverflow.com/questions/43804805/check-if-value-exists-in-enum-in-typescript

EDIT: Sorry, I know you asked me offline but I hadn't thought of that at the time 😅

So in the getter, if it's not in the enum should I throw an error?

Yeah, probably something along the lines of "Received unexpected conclusion." I'm pretty sure it'll mess up the getActionIconPath code anyway if we don't (since the path won't be found if it's not an enum we expect.)

src/tree/JobTreeItem.ts

src/tree/StepTreeItem.ts

ejizba · 2020-07-24T17:12:55Z

src/tree/JobTreeItem.ts

+    public get description(): string {
+        if (this.data.conclusion !== null) {
+            const elapsedMs: number = this.completedDate.getTime() - this.startedDate.getTime();
+            return `${convertConclusionToVerb(this.conclusion)} ${moment(this.completedDate).fromNow()} in ${prettyMs(elapsedMs)}`;


Actually, moment is writing complete phrases like 21 days ago or 21 minutes ago, so I'm not sure how we would localize that unless moment has language support.

Moment supports localization. However, I don't think "pretty-ms" does. Even if it did - my original comment was that you're putting too much stuff in the description and I stand by that. The biggest reason I love VS Code is because of how lightweight/simple it is - and I want our extensions to reflect that. Pick only the most important information to display and throw the rest in a "View Properties" command

I say we at least keep how long ago it ran (i.e. 4 days ago). I don't know if we need the verbs (like completed for instance) in there since that's kind of the point of icons (though I'm pretty sure that's not accessible, so I dunno).

I personally like the descriptions, and to your point about being lightweight/simple, while I agree, this level of description only appears if they drill down pretty far into the Actions node, in which case I feel like the user would be trying to get more detailed information about the steps.

The data object (the GitHub response type) doesn't include the times in a human friendly format (they just have completed/started timestamps) so I see some value in displaying that in a easily read manner.

I'm fine with how long ago it ran and the conclusion/status (because the icons aren't accessible, as you mentioned). The conclusion/status is most important (gotta know if it worked) and how long ago is second most important (if it ran a month ago, I might need to update my site). I just don't see as much value in the elapsed time - why do I care if it took a minute or an hour?

this level of description only appears if they drill down pretty far into the Actions node, in which case I feel like the user would be trying to get more detailed information about the steps.

so if it looks gross already - we're allowed to make it even more gross? It's not that far down :P

The data object (the GitHub response type) doesn't include the times in a human friendly format (they just have completed/started timestamps) so I see some value in displaying that in a easily read manner.

Doesn't mean you have to display it in the description. There's no reason the "properties" object we display has to be the exact object we get from GitHub. We can modify and make it look better as much as we want (still needs to be localizable, though - and out of scope for this PR).

src/tree/JobTreeItem.ts

src/tree/StepTreeItem.ts

src/tree/JobTreeItem.ts

src/tree/StepTreeItem.ts

…ticwebapps into alex/workflowJobs

alexweininger · 2020-07-29T19:28:11Z

💯

alexweininger requested a review from a team as a code owner July 20, 2020 18:02

ejizba reviewed Jul 20, 2020

View reviewed changes

src/utils/timeUtils.ts Outdated Show resolved Hide resolved

alexweininger force-pushed the alex/workflowJobs branch from d9d07f5 to ab8171c Compare July 20, 2020 21:04

Add workflow job and step tree items

cff5cf5

alexweininger force-pushed the alex/workflowJobs branch from ab8171c to cff5cf5 Compare July 20, 2020 21:12

nturinski reviewed Jul 21, 2020

View reviewed changes

alexweininger added 3 commits July 20, 2020 18:12

Use enums instead of types

b72e523

Remove parent id from id

9b667d5

Use createTreeItemsWithErrorHandling

4b9bfb8

nturinski reviewed Jul 21, 2020

View reviewed changes

src/tree/ActionTreeItem.ts Outdated Show resolved Hide resolved

ejizba requested changes Jul 21, 2020

View reviewed changes

nturinski reviewed Jul 21, 2020

View reviewed changes

src/constants.ts Outdated Show resolved Hide resolved

alexweininger added 3 commits July 21, 2020 12:31

Use pretty-ms

9109919

Move workflow types

4e3b8cd

Add completed status

cc945bb

nturinski mentioned this pull request Jul 21, 2020

Start using Octokit API #154

Merged

alexweininger added 5 commits July 22, 2020 15:14

Merge branch 'alex/workflowJobs'

81ce9a3

Use Octokit client for GitHub actions

71c9798

Merge branch 'master' of https://github.com/microsoft/vscode-azuresta…

9a6e8a7

…ticwebapps into alex/workflowJobs

Remove old types

53d6595

Move enums

bf1bf6b

nturinski reviewed Jul 24, 2020

View reviewed changes

src/tree/ActionsTreeItem.ts Show resolved Hide resolved

nturinski reviewed Jul 24, 2020

View reviewed changes

src/tree/JobTreeItem.ts Outdated Show resolved Hide resolved

Fix id

c9a2272

nturinski reviewed Jul 24, 2020

View reviewed changes

src/tree/StepTreeItem.ts Outdated Show resolved Hide resolved

Getters for status and conclusion

2fb34a0

ejizba reviewed Jul 24, 2020

View reviewed changes

Merge branch 'master' of https://github.com/microsoft/vscode-azuresta…

d4ae6f3

…ticwebapps into alex/workflowJobs

nturinski and others added 4 commits July 29, 2020 10:55

PR feedback

421f1ec

Run npm install to clean pretty-ms from lock

d4e8291

Ensure type saftely of status/conclusion

1773cd0

Clean up some duplicate code

4533661

ejizba approved these changes Jul 29, 2020

View reviewed changes

nturinski approved these changes Jul 29, 2020

View reviewed changes

nturinski closed this Jul 29, 2020

nturinski reopened this Jul 29, 2020

Merge branch 'master' into alex/workflowJobs

f6a9741

nturinski merged commit a0e17d4 into master Jul 29, 2020

nturinski deleted the alex/workflowJobs branch July 29, 2020 23:14

github-actions bot locked and limited conversation to collaborators Feb 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show workflow jobs and steps #152

Show workflow jobs and steps #152

alexweininger commented Jul 20, 2020

nturinski Jul 21, 2020

alexweininger Jul 21, 2020

ejizba Jul 21, 2020

ejizba Jul 21, 2020

ejizba Jul 21, 2020

nturinski Jul 24, 2020

alexweininger Jul 24, 2020

nturinski Jul 24, 2020 •

edited

nturinski Jul 24, 2020 •

edited

alexweininger Jul 24, 2020

nturinski Jul 24, 2020

ejizba Jul 24, 2020

nturinski Jul 24, 2020

ejizba Jul 24, 2020

alexweininger commented Jul 29, 2020


		// Doc for these parameter values: https://developer.github.com/v3/checks/runs/#parameters

		export enum Conclusion {

Show workflow jobs and steps #152

Show workflow jobs and steps #152

Conversation

alexweininger commented Jul 20, 2020

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

nturinski Jul 24, 2020 • edited

Choose a reason for hiding this comment

nturinski Jul 24, 2020 • edited

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

alexweininger commented Jul 29, 2020

nturinski Jul 24, 2020 •

edited

nturinski Jul 24, 2020 •

edited