Skip to content
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

Adding timeout options both CLI and .cigar.json #32

Merged
merged 4 commits into from
Mar 12, 2019

Conversation

WyriHaximus
Copy link
Collaborator

🎆🎆🎆 Happy New Year 🎆🎆🎆

This PR adds the timeout options discussed in #31
Closes / implements #31

Any values in the CLI are overridden by what ever value is in the .cigar.json

🎆🎆🎆 Happy New Year 🎆🎆🎆

This PR adds the timeout options discussed in Brunty#31
Closes / implements Brunty#31

Any values in the CLI are overridden by what ever value is in the `.cigar.json`
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e44ac97 on WyriHaximus-labs:timeouts into b8d82fb on Brunty:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e44ac97 on WyriHaximus-labs:timeouts into b8d82fb on Brunty:master.

@coveralls
Copy link

coveralls commented Dec 31, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 56cba08 on WyriHaximus-labs:timeouts into b8d82fb on Brunty:master.

Copy link
Owner

@Brunty Brunty left a comment

Choose a reason for hiding this comment

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

Needs documentation of how this is configured adding to README

bin/cigar Outdated
@@ -18,7 +18,7 @@ use Brunty\Cigar\Outputter;
use Brunty\Cigar\Parser;
use Brunty\Cigar\Result;

$options = getopt('c:ia:u:jh:', ['version', 'help', 'quiet', 'config:', 'insecure', 'auth:', 'url:', 'json', 'header:']);
$options = getopt('c:ia:u:jh:t:ct:', ['version', 'help', 'quiet', 'config:', 'insecure', 'auth:', 'url:', 'json', 'header:', 'timeout:', 'connect-timeout:']);
Copy link
Owner

Choose a reason for hiding this comment

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

Shortopts are for single-character options only, so ct won't work (we also already have a c and a t option, so it'll produce results like this:

> php opts.php -c 'config_file.json' -t 'timeout' -ct '1'
array(2) {
  ["c"]=>
  array(2) {
    [0]=>
    string(16) "config_file.json"
    [1]=>
    string(1) "t"
  }
  ["t"]=>
  string(7) "timeout"
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doh my bad, changed it 👍

bin/cigar Outdated
\033[32m-i, --insecure\033[0m Allow invalid SSL certificates
\033[32m-a, --auth\033[0m Authorization header "\074type\076 \074credentials\076"
\033[32m-h, --header\033[0m Custom header "\074name\076: \074value\076"
\033[32m-ct, --connect-timeout=TIMEOUT\033[0m Connect Timeout
Copy link
Owner

Choose a reason for hiding this comment

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

Help output will need changing with the change of the ct option

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the mention of ct here

spec/AsyncCheckerSpec.php Show resolved Hide resolved
Copy link
Owner

@Brunty Brunty left a comment

Choose a reason for hiding this comment

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

Wondering what we do about the connect-timeout stuff now? 🤔

bin/cigar Outdated
$secure = ! (isset($options['i']) || isset($options['insecure']));
$authorization = $options['a'] ?? $options['auth'] ?? null;
$headers = (array) ($options['h'] ?? $options['header'] ?? []);
$connectTimeout = $options['connect-timeout'] ?? null;
Copy link
Owner

Choose a reason for hiding this comment

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

This will always be null as connect-timeout is not in the options anymore (but it's still in the help text)

@WyriHaximus
Copy link
Collaborator Author

@Brunty pick a letter and I can use that one. But there isn't one that directly makes sense.

@WyriHaximus
Copy link
Collaborator Author

@Brunty ping, let me know if you have a preference for a letter I can use, otherwise I'll pick one using RNG later this week

@WyriHaximus
Copy link
Collaborator Author

@Brunty ok updated it with s as short for connect-timeout.

@WyriHaximus
Copy link
Collaborator Author

Assuming this would be 1.12, should I tag is later today?

@Brunty Brunty merged commit 6389b9e into Brunty:master Mar 12, 2019
@Brunty
Copy link
Owner

Brunty commented Mar 12, 2019

Please! I'll squash and merge now :)

@WyriHaximus
Copy link
Collaborator Author

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

Successfully merging this pull request may close these issues.

3 participants