Add Duration property to HistoryInfo by powercode · Pull Request #5208 · PowerShell/PowerShell

Staffan Gustafsson (powercode) · 2017-10-24T08:45:17Z

Fixes #4181
Adding a property Duration to HistoryInfo to end the {$_.EndExecutionTime - $_.StartExecutionTime} madness :)

This change is

sergei (vors)

sweet! any tests?

Staffan Gustafsson (powercode) · 2017-10-27T13:52:28Z

Not really sure what to test. Testing

public TimeSpan Duration => EndExecutionTime - StartExecutionTime;

will hardly give a lot.
But maybe a test that the formatting view is in place?

sergei (vors) · 2017-10-28T06:06:25Z

Not really sure what to test

GetExecutionTimeString seems like a good candidate for unit tests

Ilya (iSazonov) · 2017-11-08T12:53:16Z

Staffan Gustafsson (@powercode) Please resolve merge conflicts.

Staffan Gustafsson (powercode) · 2017-11-08T18:55:13Z

sergei (@vors) You were so right. Added tests and fixed bugs

sergei (vors)

Looks solid, but not a big fan of reflection in the tests. This is very fragile.
I guess it should not be a blocker, but maybe we can just do some unit testing from C#? Don't we have such tests?

Ilya (iSazonov)

Leave a comment

Ilya (iSazonov) · 2017-11-10T05:00:57Z

Should we remove the test?

Ilya (iSazonov) · 2017-11-10T05:02:31Z

TimeSeparator can be culture sensitive. Could you please use TimeSpan constructor for duration?

Also in C# $@"{duration:d\.hh\:mm\:ss}"; should we be culture sensitive too?

Ilya (@iSazonov) Excellent catch!
I will fix!

We keep the implementation as it is.
Closed.

Steve Lee (SteveL-MSFT) · 2017-11-15T17:49:48Z

We're not holding the RC for this PR, if it makes it in soon, great! If not, it'll catch the next release.

mi-hol · 2017-12-14T19:59:55Z

Steve Lee (@SteveL-MSFT) will it merge this time?

Steve Lee (SteveL-MSFT) · 2017-12-15T00:32:59Z

mi-hol if the maintainer Ilya (@iSazonov) believes there has been sufficient review, he can merge this, but we're limiting code changes we're taking for GA to avoid risk of regression. This will show up in 6.1.0 preview release which should happen early next year.

Ilya (iSazonov) · 2017-12-15T09:23:39Z

We have only one open comment about TimeSeparator.
I don't see any reason why we won't get it in 6.1.0.

Staffan Gustafsson (powercode) · 2018-01-24T22:36:40Z

I have looked a bit on how PowerShell treats timespans in other cases, and it seems to use the default formatting, which is not culture sensitive.
I think we should stick with consistency here, and keep the implementation as it is.

Ilya (iSazonov) · 2018-02-21T16:15:24Z

Staffan Gustafsson (@powercode) please resolve conflicts.

Ilya (iSazonov) · 2018-02-22T04:56:08Z

Steve Lee (@SteveL-MSFT) Dongbo Wang (@daxian-dbw) Have you any thoughts about the PR?

Steve Lee (SteveL-MSFT)

Regarding GetDurationString() vs ToDurationString(), looking in PS code, it seems Get*String() is used (although internal methods). I'm fine keeping GetDurationString() unless someone can point to some guidance otherwise.

Ilya (iSazonov) · 2018-02-24T05:55:28Z

Travis Plunk (@TravisEz13) Dongbo Wang (@daxian-dbw) Have you any thoughts about GetDurationString() vs ToDurationString()?

This adds the property ExecutionTime to the table view

Renaming GetExecutionTimeString() -> GetDurationString()

Staffan Gustafsson (powercode) · 2018-07-04T12:40:48Z

These build errors are not related to my changes.

Can anyone restart the build here?

Ilya (iSazonov) · 2018-07-04T13:32:58Z

Reopen the PR to restart CIs.

Changes were recommended after approval

Ilya (iSazonov) · 2018-07-17T06:04:08Z

Staffan Gustafsson (@powercode) Is the PR ready for review?

Staffan Gustafsson (powercode) · 2018-07-18T16:14:23Z

Yes

Ilya (iSazonov) · 2018-07-19T03:18:40Z

Dongbo Wang (@daxian-dbw) Steve Lee (@SteveL-MSFT) Bruce Payette (@BrucePay) Please review the PR.

Travis Plunk (TravisEz13)

Reviewed 1 of 1 files at r4, 1 of 2 files at r5.
Reviewable status: complete! all files reviewed

Steve Lee (SteveL-MSFT) · 2018-07-21T14:43:45Z

    }
+
+    It "HistoryInfo calculates Duration" {
+        $ctor = [Microsoft.PowerShell.Commands.HistoryInfo].GetConstructors([Reflection.BindingFlags]::NonPublic -bor [Reflection.BindingFlags]::Instance).Where{$_.GetParameters().Length -eq 5}


Seems like this test should just get the first item from item from Get-History and validate that Duration = End - Start?

Steve Lee (@SteveL-MSFT) Am I guaranteed to have something in the history at that moment?

Staffan Gustafsson (@powercode) I think it's ok to use Add-History

Steve Lee (SteveL-MSFT)

LGTM

Ilya (iSazonov) · 2018-08-27T07:05:16Z

+        }
+        $history | Add-History
+        $h = Get-History -count 1
+        $h.Duration | Should Be $duration


Staffan Gustafsson (@powercode) Sorry I lost the PR. Please fix Be -> -Be and we ready to merge.

Ilya (iSazonov) · 2018-08-28T03:05:32Z

Staffan Gustafsson (@powercode) Thank you for the contribution! Sorry for delay.

* Remove regions in HistoryInfo class * Auto properties refactoring in HistoryInfo * Adding Duration property to HistoryInfo * Rename ExecutionTime => Duration

Staffan Gustafsson (powercode) requested review from Bruce Payette (BrucePay), Dongbo Wang (daxian-dbw), Jason Shirk (lzybkr) and sergei (vors) as code owners October 24, 2017 08:45

sergei (vors) reviewed Oct 25, 2017

View reviewed changes

Ilya (iSazonov) self-assigned this Oct 25, 2017

Staffan Gustafsson (powercode) force-pushed the historyInfo branch from a4e8808 to f0674cc Compare November 8, 2017 18:54

sergei (vors) approved these changes Nov 9, 2017

View reviewed changes

Ilya (iSazonov) reviewed Nov 9, 2017

View reviewed changes

Staffan Gustafsson (powercode) changed the title ~~Add ExecutionTime property to HistoryInfo~~ Add Duration property to HistoryInfo Nov 9, 2017

Ilya (iSazonov) reviewed Nov 10, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) modified the milestones: 6.0.0-RC, 6.0.0-NiceToHave Nov 11, 2017

Steve Lee (SteveL-MSFT) removed this from the 6.0.0-NiceToHave milestone Dec 15, 2017

Staffan Gustafsson (powercode) force-pushed the historyInfo branch from 601c66c to 89ca944 Compare February 21, 2018 20:59

Steve Lee (SteveL-MSFT) previously approved these changes Feb 22, 2018

View reviewed changes

Staffan Gustafsson and others added 8 commits June 18, 2018 22:34

Auto properties refactoring in HistoryInfo

3afd374

Adding ExecutionTime property to HistoryInfo

1a2b81e

Adding formatting table view "time" for historyInfo

82e8b7f

This adds the property ExecutionTime to the table view

Rename ExecutionTime => Duration

c89be3a

Adding test for time table view

bea8bd5

Adding tests

34cc89a

Renaming GetExecutionTimeString() -> GetDurationString()

Single line methods to expression bodys

3a42a4b

Removing time view

84f6949

Staffan Gustafsson (powercode) force-pushed the historyInfo branch from 7720c1c to 84f6949 Compare June 18, 2018 21:05

Removing obsolete test

10c882d

Staffan Gustafsson (powercode) changed the title ~~WIP: Add Duration property to HistoryInfo~~ Add Duration property to HistoryInfo Jun 19, 2018

Ilya (iSazonov) closed this Jul 4, 2018

Ilya (iSazonov) reopened this Jul 4, 2018

Travis Plunk (TravisEz13) reviewed Jul 20, 2018

View reviewed changes

Steve Lee (SteveL-MSFT) requested changes Jul 21, 2018

View reviewed changes

Using Add-History to test Duration property

666a425

Steve Lee (SteveL-MSFT) approved these changes Jul 31, 2018

View reviewed changes

Ilya (iSazonov) reviewed Aug 27, 2018

View reviewed changes

Be -> -Be

5e5af80

Staffan Gustafsson (powercode) force-pushed the historyInfo branch from c2a1537 to 5e5af80 Compare August 27, 2018 19:48

Ilya (iSazonov) merged commit 1bfe96d into PowerShell:master Aug 28, 2018

Travis Plunk (TravisEz13) added this to the v6.1.0 milestone Aug 28, 2018

Uh oh!

Conversation

Staffan Gustafsson (powercode) commented Oct 24, 2017 • edited by adityapatwardhan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergei (vors) left a comment

Choose a reason for hiding this comment

Uh oh!

Staffan Gustafsson (powercode) commented Oct 27, 2017

Uh oh!

sergei (vors) commented Oct 28, 2017

Uh oh!

Ilya (iSazonov) commented Nov 8, 2017

Uh oh!

Staffan Gustafsson (powercode) commented Nov 8, 2017

Uh oh!

sergei (vors) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) 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!

Ilya (iSazonov) 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!

Ilya (iSazonov) Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Nov 15, 2017

Uh oh!

mi-hol commented Dec 14, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Dec 15, 2017

Uh oh!

Ilya (iSazonov) commented Dec 15, 2017

Uh oh!

Staffan Gustafsson (powercode) commented Jan 24, 2018

Uh oh!

Ilya (iSazonov) commented Feb 21, 2018

Uh oh!

Ilya (iSazonov) commented Feb 22, 2018

Uh oh!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Feb 24, 2018 • edited by TravisEz13 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Staffan Gustafsson (powercode) commented Jul 4, 2018

Uh oh!

Ilya (iSazonov) commented Jul 4, 2018

Uh oh!

Ilya (iSazonov) commented Jul 17, 2018

Uh oh!

Staffan Gustafsson (powercode) commented Jul 18, 2018

Uh oh!

Ilya (iSazonov) commented Jul 19, 2018

Uh oh!

Travis Plunk (TravisEz13) left a comment

Choose a reason for hiding this comment

Staffan Gustafsson (powercode) commented Oct 24, 2017 •

edited by adityapatwardhan

Loading

sergei (vors) left a comment •

edited

Loading

Ilya (iSazonov) Jan 25, 2018 •

edited

Loading

Ilya (iSazonov) commented Feb 24, 2018 •

edited by TravisEz13

Loading