New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Parameter: Start-Sleep [-Duration] <timespan> #16185
New Parameter: Start-Sleep [-Duration] <timespan> #16185
Conversation
My understanding of PowerShell Committee conclusion in #12305 (comment) is that we need to implement something like Start-Sleep 3sec
Start-Sleep 1.5min The PR is not follow the conclusion. |
@iSazonov In order for that to ever work consistently, this change is needed. Without an option for binding
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
src/Microsoft.PowerShell.Commands.Utility/commands/utility/StartSleepCommand.cs
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/StartSleepCommand.cs
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/StartSleepCommand.cs
Outdated
Show resolved
Hide resolved
…rtSleepCommand.cs Co-authored-by: Dongbo Wang <dongbow@microsoft.com>
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment. |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
src/Microsoft.PowerShell.Commands.Utility/resources/StartSleepStrings.resx
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/resources/StartSleepStrings.resx
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Start-Sleep.Tests.ps1
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IISResetMe Can you please address the CodeFactor issues? No need to address all, but just those issue related to your new changes.
src/Microsoft.PowerShell.Commands.Utility/commands/utility/StartSleepCommand.cs
Outdated
Show resolved
Hide resolved
…rtSleepCommand.cs Co-authored-by: Dongbo Wang <dongbow@microsoft.com>
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Thanks @daxian-dbw, I believe you already fixed all the relevant ones 😅 - the remaining issues all complain about documentation and I ignored those on purpose since it would break with the description convention in the existing comment docs (the property summaries all start with "Allows sleep time to be ..."). I'm not sure why it says "1 issue", when I click through there's 5 and they all appear to be non-critical |
I guess the other 4 are known to CodeRefactor and the remaining one is new :) |
That must be the one for the property I added then, all good!
That's exactly right! The other half of the puzzle in #12305 is implementing tokenization and parsing logic for timespan literals so we can do:
I intend to pick this up too :) |
🎉 Handy links: |
PR Context
As originally propoesd in #12305, this PR adds a new
-Duration
parameter to theStart-Sleep
cmdlet:The existing parameter sets of
Start-Sleep
remain the same, and I have not changed theDefaultParameterSetName
, but the pipeline binding does interferes with the existing (buggy/counter-intuitive) binding behavior:This PR also adds range validation support for
[timespan]
values via[ValidateRange()]
:Strict range validation is not of much immediate use, but in a possible future with timespan literals we could do:
PR Context
I believe this change is reasonable to make without wrapping in an Experimental Feature - the behavior that we're breaking ("piped
[timespan]
values are interpreted as seconds % 60") is rather counter-intuitive and deceptive.PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).