GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Start-Sleep: add 'ms' alias by Tadas · Pull Request #4039 · PowerShell/PowerShell

Conversation

@Tadas

@Tadas Tadas Medišauskas (Tadas) commented Jun 17, 2017

Copy link
Copy Markdown
Contributor

Fixes #3991

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mklement0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@Tadas
Start-Sleep: add 'ms' alias
e75fee1
@msftclas

Microsoft Contribution License Agreements (msftclas) commented Jun 17, 2017

Copy link
Copy Markdown

@Tadas,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-required label Jun 17, 2017
@msftclas

Microsoft Contribution License Agreements (msftclas) commented Jun 17, 2017

Copy link
Copy Markdown

Tadas Medišauskas (@Tadas), thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@msftclas Microsoft Contribution License Agreements (msftclas) added cla-signed and removed cla-required labels Jun 17, 2017
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Jun 17, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Start-Sleep.Tests.ps1
$watch = [System.Diagnostics.Stopwatch]::StartNew()
Start-Sleep -ms 1000
$watch.Stop()
$watch.ElapsedMilliseconds | Should BeGreaterThan 950

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jun 17, 2017

Copy link
Copy Markdown
Member

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.

Seems like you should also make sure it's less than 1050

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@iSazonov Ilya (iSazonov) Jun 17, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.

Should we test parameter aliases? It seems it is a general feature and we should test the feature in special tests but not every cmdlet and every alias.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jun 17, 2017

Copy link
Copy Markdown
Member

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.

In general, we should validate the alias is correct, but shouldn't repeat every test for the parameter and its alias. -ms is a pretty simple case, so it's hard to not repeat the test (using -TestCases is probably better here). For parameters with more complex behavior, I think it would be sufficient if some cases used the parameter while others used the alias.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@lzybkr Jason Shirk (lzybkr) Jun 18, 2017

Copy link
Copy Markdown
Contributor

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.

Ilya (@iSazonov) - As a matter of principle - we avoid testing the parameter binder in cmdlet specific tests.

Personally I don't ask for any tests for an alias - the test isn't covering any new code, but I understand the concerns of those asking for the test, so I don't complain about it either.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Jun 19, 2017

Copy link
Copy Markdown
Collaborator

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.

If we're worried about parameter's aliases, we could make a common test for all of them, just like we did for the cmdlets and their aliases as < cmdlet, parameter, alias, platform(?) >.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@lzybkr Jason Shirk (lzybkr) Jun 19, 2017

Copy link
Copy Markdown
Contributor

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.

It's not a big problem, people mostly never remove code anyway and if they did, it should be caught during the PR, but it's a nice idea.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@Tadas
Add BeLessThan tests
2a42f0b
@Tadas

Tadas Medišauskas (Tadas) commented Jun 17, 2017

Copy link
Copy Markdown
Contributor Author

What about the Describe "Start-Sleep" -Tags "CI" { test, isn't it redundant?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Jun 19, 2017
View reviewed changes
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit 437d773 into PowerShell:master Jun 19, 2017
@iSazonov

Ilya (iSazonov) commented Jun 20, 2017

Copy link
Copy Markdown
Collaborator

Tadas Medišauskas (@Tadas) Thanks for your contribution!

SteveL-MSFT reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Tadas
Tadas Medišauskas (Tadas) deleted the start-sleep-ms-alias branch June 25, 2017 19:38
@mklement0 Michael Klement (mklement0) mentioned this pull request Apr 25, 2019
Dash as a parameter crashes PWSH #9362
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@Tadas @daxian-dbw
Start-Sleep: add 'ms' alias to '-Milliseconds' (PowerShell#4039)
57a0d2a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
+1 more reviewer
@lzybkr Jason Shirk (lzybkr) lzybkr left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@Tadas @msftclas @iSazonov @lzybkr @SteveL-MSFT @daxian-dbw
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.