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

Fix for #1104 (configure-integration-tests.ps1) #1118

Merged
merged 2 commits into from
Mar 8, 2016

Conversation

nubby109
Copy link
Contributor

@nubby109 nubby109 commented Mar 6, 2016

Made changes in .\script\configure-integration-tests.ps1.
For an optional value it gives the option to change or remove the value, for a necessary value it doesn't give an option.

Fixes #1104

Made changes in configure-integration-tests.ps1. For an optional value
it gives the option to change or remove the value, for a necessary value
it doesn't give an option.
@shiftkey
Copy link
Member

shiftkey commented Mar 7, 2016

While we're in here, could you look at making the whitespace consistent? It's probably my fault that there's some inconsistencies in here...

}
else
{
$reset = Read-Host -Prompt "Want to change this value , press Y, otherwise we'll move on"
Copy link
Member

Choose a reason for hiding this comment

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

Let's tweak this message: Press Y to change this value, otherwise we'll move on

@shiftkey
Copy link
Member

shiftkey commented Mar 7, 2016

@Anubhav10 this looks good - just that one improvement to the message and some cleanup of the whitespace and I think this is good to go!

@@ -1,4 +1,4 @@
# TODO: this should indicate whether a variable is required or optional
# TODO: this should indicate whether a variable is required or optional
Copy link
Member

Choose a reason for hiding this comment

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

This is an old comment from before I actually implemented the optional parameters. Feel free to remove this 🔥

@nubby109
Copy link
Contributor Author

nubby109 commented Mar 7, 2016

I'll do the suggested changes. Yesterday, the Travis CI build was failing, how come it passed today? I tried to understand why it was failing but I couldn't.

@ryangribble
Copy link
Contributor

Unfortunately we are sometimes seeing unrelated failures on the linux and/or OSX travis CI builds.
#1076

made white spaces consistent and changed the message
@nubby109
Copy link
Contributor Author

nubby109 commented Mar 7, 2016

@shiftkey The whitespace inconsistency was in my code and was my fault. I have removed the TODO comment and changed the message.

@shiftkey
Copy link
Member

shiftkey commented Mar 8, 2016

@Anubhav10 excellent, thanks for this!

shiftkey added a commit that referenced this pull request Mar 8, 2016
@shiftkey shiftkey merged commit c4d6cd5 into octokit:master Mar 8, 2016
@ryangribble ryangribble changed the title Fixed Issue 1104 Fix for #1104 (configure-integration-tests.ps1) Mar 14, 2016
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.

3 participants