Skip to content

Conversation

@jpsnover
Copy link
Contributor

This replaces PR #2479

@msftclas
Copy link

Hi @jpsnover, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

@JamesWTruher
Copy link
Collaborator

jeffrey and I spoke about my comments - the current timeout is 1000 ms, so this does not change our current behavior and provides for more flexibility

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment does not seem to apply

@JamesWTruher
Copy link
Collaborator

I'd love to see additional tests here -

Describe "test-connection" -Tags @('Feature','Slow' ) {
     It "test-connection respects timeout" {
        $start = [datetime]::now
        Test-Connection -Computer _bad_computer_name_ -Timeout 10000 -ea silentlycontinue
        $end = [datetime]::now
        ($end - $start).TotalSeconds | should BeGreaterThan 10.0
     }
}

something along those lines

@andyleejordan
Copy link
Member

andyleejordan commented Oct 17, 2016

Your committer email (private@jsnover.com) appears to be connected to the @jsnover GitHub account, and not the @jpsnover GitHub account you opened the PR from (so the commit doesn't "belong" to @jpsnover as far as GitHub is concerned). Do you mean to use this email for you commits, and if so, is it possible to verify it with your current @jpsnover GitHub account?

@jpsnover
Copy link
Contributor Author

@andschwa I've made it so hat private@jsnover.com is the only account connected to my jpsnover github account.
let me know if I need to do anything else

/// The following is the definition of the input parameter "TimeOut".
/// Time-out value in milliseconds. If a response is not received in this time, no response is assumed. The default is 1000 milliseconds.
/// </summary>
[Parameter]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never liked this use of ValidateRange - it seems like we should have ValidatePositive and ValidateNonNegative attributes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.