Add Duration property to HistoryInfo #5208

powercode · 2017-10-24T08:45:17Z

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

This change is

vors

sweet! any tests?

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?

vors · 2017-10-28T06:06:25Z

Not really sure what to test

GetExecutionTimeString seems like a good candidate for unit tests

iSazonov · 2017-11-08T12:53:16Z

@powercode Please resolve merge conflicts.

powercode · 2017-11-08T18:55:13Z

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

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?

iSazonov

Leave a comment

iSazonov · 2017-11-10T05:00:57Z

test/powershell/Modules/Microsoft.PowerShell.Core/History.Tests.ps1

Should we remove the test?

iSazonov · 2017-11-10T05:02:31Z

test/powershell/Modules/Microsoft.PowerShell.Core/History.Tests.ps1

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?

@iSazonov Excellent catch!
I will fix!

We keep the implementation as it is.
Closed.

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

@SteveL-MSFT will it merge this time?

SteveL-MSFT · 2017-12-15T00:32:59Z

@mi-hol if the maintainer @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.

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.

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.

iSazonov · 2018-02-21T16:15:24Z

@powercode please resolve conflicts.

iSazonov · 2018-02-22T04:56:08Z

@SteveL-MSFT @daxian-dbw Have you any thoughts about the PR?

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.

iSazonov · 2018-02-24T05:55:28Z

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

This adds the property ExecutionTime to the table view

Renaming GetExecutionTimeString() -> GetDurationString()

powercode · 2018-07-04T12:40:48Z

These build errors are not related to my changes.

Can anyone restart the build here?

iSazonov · 2018-07-04T13:32:58Z

Reopen the PR to restart CIs.

Changes were recommended after approval

iSazonov · 2018-07-17T06:04:08Z

@powercode Is the PR ready for review?

powercode · 2018-07-18T16:14:23Z

Yes

iSazonov · 2018-07-19T03:18:40Z

@daxian-dbw @SteveL-MSFT @BrucePay Please review the PR.

TravisEz13

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

SteveL-MSFT · 2018-07-21T14:43:45Z

test/powershell/Modules/Microsoft.PowerShell.Core/History.Tests.ps1

    }
+
+    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?

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

@powercode I think it's ok to use Add-History

SteveL-MSFT

LGTM

iSazonov · 2018-08-27T07:05:16Z

test/powershell/Modules/Microsoft.PowerShell.Core/History.Tests.ps1

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


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

iSazonov · 2018-08-28T03:05:32Z

@powercode Thank you for the contribution! Sorry for delay.

powercode requested review from BrucePay, daxian-dbw, lzybkr and vors as code owners October 24, 2017 08:45

vors reviewed Oct 25, 2017

View reviewed changes

iSazonov self-assigned this Oct 25, 2017

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

vors approved these changes Nov 9, 2017

View reviewed changes

iSazonov reviewed Nov 9, 2017

View reviewed changes

powercode changed the title ~~Add ExecutionTime property to HistoryInfo~~ Add Duration property to HistoryInfo Nov 9, 2017

iSazonov reviewed Nov 10, 2017

View reviewed changes

SteveL-MSFT modified the milestones: 6.0.0-RC, 6.0.0-NiceToHave Nov 11, 2017

SteveL-MSFT removed this from the 6.0.0-NiceToHave milestone Dec 15, 2017

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

SteveL-MSFT previously approved these changes Feb 22, 2018

View reviewed changes

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

Remove regions in HistoryInfo class

8a4d5cb

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

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

Removing obsolete test

10c882d

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

iSazonov closed this Jul 4, 2018

iSazonov reopened this Jul 4, 2018

TravisEz13 reviewed Jul 20, 2018

View reviewed changes

SteveL-MSFT requested changes Jul 21, 2018

View reviewed changes

Using Add-History to test Duration property

666a425

SteveL-MSFT approved these changes Jul 31, 2018

View reviewed changes

iSazonov reviewed Aug 27, 2018

View reviewed changes

Be -> -Be

5e5af80

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

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

TravisEz13 added this to the v6.1.0 milestone Aug 28, 2018

Add Duration property to HistoryInfo #5208

Add Duration property to HistoryInfo #5208

Uh oh!

Conversation

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!

vors left a comment

Choose a reason for hiding this comment

Uh oh!

powercode commented Oct 27, 2017

Uh oh!

vors commented Oct 28, 2017

Uh oh!

iSazonov commented Nov 8, 2017

Uh oh!

powercode commented Nov 8, 2017

Uh oh!

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!

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!

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!

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!

SteveL-MSFT commented Nov 15, 2017

Uh oh!

mi-hol commented Dec 14, 2017

Uh oh!

SteveL-MSFT commented Dec 15, 2017

Uh oh!

iSazonov commented Dec 15, 2017

Uh oh!

powercode commented Jan 24, 2018

Uh oh!

iSazonov commented Feb 21, 2018

Uh oh!

iSazonov commented Feb 22, 2018

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

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!

powercode commented Jul 4, 2018

Uh oh!

iSazonov commented Jul 4, 2018

Uh oh!

iSazonov commented Jul 17, 2018

Uh oh!

powercode commented Jul 18, 2018

Uh oh!

iSazonov commented Jul 19, 2018

Uh oh!

powercode commented Oct 24, 2017 •

edited by adityapatwardhan

Loading

vors left a comment •

edited

Loading

iSazonov Jan 25, 2018 •

edited

Loading

iSazonov commented Feb 24, 2018 •

edited by TravisEz13

Loading