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

Make the —yes/y arg set all confirmations to yes #1201

Merged
merged 4 commits into from Sep 14, 2016
Merged

Conversation

ronan
Copy link
Contributor

@ronan ronan commented Sep 14, 2016

@greg-1-anderson, @TeslaDethray This works bit is kinda simplistic. It differs from --no-interaction in that it only affects confirmations and defaults to 'y' and not 'n'.

@pantheon-mentionbot
Copy link

@ronan, thanks for your PR! By analyzing the blame information on this pull request, we identified @TeslaDethray and @greg-1-anderson to be potential reviewers

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage remained the same at 14.05% when pulling 28b4818 on addition/confirm-yes into 3584938 on master.

@greg-1-anderson
Copy link
Member

Sure, works for me; however, I think that we should also make --yes imply --no-interaction. Anything that is not confirm() will return the default value, which I think is correct for --yes.

@ronan
Copy link
Contributor Author

ronan commented Sep 14, 2016

@TeslaDethray Was saying the opposite. I'm agnostic. Let's merge this as is and re-address with product/design when we get to command for which this matters

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage remained the same at 14.05% when pulling 31176bc on addition/confirm-yes into da4f14b on master.

@greg-1-anderson
Copy link
Member

Agree we should leave that out until certain about how we want it.

@greg-1-anderson
Copy link
Member

I'm in favor of merging here.


/**
* @param $question
* @return string
Copy link
Contributor

Choose a reason for hiding this comment

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

Return is not always a string.

@@ -37,4 +38,16 @@ protected function log()
{
return $this->logger;
}

/**
* @param $question
Copy link
Contributor

Choose a reason for hiding this comment

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

No var type

@@ -37,4 +38,16 @@ protected function log()
{
return $this->logger;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

No description

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Changes Unknown when pulling 496c398 on addition/confirm-yes into * on master*.

@TeslaDethray
Copy link
Contributor

I strongly dislike the idea of handling interactivity from inside these commands. If we wish to retain --yes, -y, it would be better to implement them on an as-needed basis.

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage remained the same at 14.05% when pulling 496c398 on addition/confirm-yes into 41dcd1d on master.

@ronan ronan merged commit ee53b86 into master Sep 14, 2016
@ronan ronan deleted the addition/confirm-yes branch September 14, 2016 23:14
@greg-1-anderson
Copy link
Member

I am misunderstanding one of your two sentences.

I strongly dislike the idea of handling interactivity from inside these commands

Meaning you prefer to have a confirm() function as is presented in this PR?

it would be better to implement them on an as-needed basis.

Meaning you'd prefer to put a confirm() function in each command?

I must be mis-parsing one of those.

ronan pushed a commit that referenced this pull request Sep 15, 2016
…rm-yes"

This reverts commit ee53b86, reversing
changes made to 41dcd1d.
TeslaDethray added a commit that referenced this pull request Sep 16, 2016
Revert "Merge pull request #1201 from pantheon-systems/addition/confi…
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.

None yet

5 participants