#185 Remove default choice from InteractivePrompt.prompt_for_confirmation and all associated tests

[christianrondeau]

Resolves #185 (Do not offer a default option when prompting for a user choice).

Note that it simply removes the code in interactivePrompt for defaultValue (first commit) and requireValue (second commit) since nowhere in the code other than in tests was the value actually optional. Most of the changes other than that are tests that don't make sense anymore (e.g. all tests for optional/default prompts).

This is my first pull request for this project, so feel free to suggest changes to how I make these (e.g. leave all tests and ask), since I plan on doing a few more. All tests are green!

[gep13]

@christianrondeau can I get you to have a read of our Contributing document, specifically this section:

https://github.com/chocolatey/choco/blob/master/CONTRIBUTING.md#prepare-commits

Thanks

[christianrondeau]

@gep13 I did, and carefully :) I was unsure of my pull request especially because of all removed associated tests. I guess this is why you are referring me to the CONTRIBUTING.md file.

Since the issue was about removing the defaults everywhere, I thought of removing the functionality instead of changing only usages; many tests were written specifically to handle defaults; I did a minimal cleanup (which happens to be quite a lot). I also saw that requireValue (which is associated with defaultValue) was actually not used, and was also phantom code. I made a separate commit for that, which can be rolled back.

So, I had two choices: either I only changed the calls to prompt_for_confirmation and passed null everywhere (leaving lots phantom code and tests), or I cleaned up after myself and removed code that was not used anymore.

I usually preferred the latter, but if you prefer the former, I can certainly reissue a pull request and do that in the future. Let me know!

[gep13]

The main thing that I was referring to was this:

The first line of the commit message should be a short description around 50 characters in length and be prefixed with the GitHub issue it refers to with parentheses surrounding that. If the GitHub issue is #25, you should have (GH-25) prefixed to the message.

As I noticed that the commit message was wrapping around:

I am not yet clear on how stringent @ferventcoder is going to be about things like this 😸

[gep13]

I hadn't actually looked at the contents of the PR from a review perspective, I was just trying to pre-empt a possible issue that you might be called on 😸

[ferventcoder]

My biggest issue is the removing of code from infrastructure. The infrastructure section is general purpose items that could be pulled out and into other things or their own things.
Infrastructure.app is application-specific, tailored things for the application in question, where these specific changes would have made more sense to destroy features.

I'd rather see adding additional items to the general purpose area. Almost all of the work here could have been done by simple changes to the parameters passed.

One could add an application specific prompt setup that the app would call instead that will call the general purpose one with the specific setup, if that makes sense.

[christianrondeau]

@gep: Thanks, I'll re-issue the commits with shorter names.

@ferventcoder: So you are saying: keep the "unused" code, as it is infrastructure code and may be used for other purposes in the future. Is this correct? I would not create an application-specific wrapper just for this then, I'd simply pass null to default value and false to required, it will make a much smaller commit :)

I'll try to avoid "cleaning up" after myself in future pull requests when the code is in the infrastructure section, but still do it for code in the app-specific portion of the code. Let me know if I misunderstood anything, let's make this "simple" PR perfect before I start piling up other PRs :)

PR coming up in a few hours (when I have some free time)

[ferventcoder]

So you are saying: keep the "unused" code, as it is infrastructure code and may be used for other purposes in the future. Is this correct? I would not create an application-specific wrapper just for this then, I'd simply pass null to default value and false to required, it will make a much smaller commit :)

Yes, I think you have stated that correctly. I think you'd still want required: true, as that is what it was before and that's how you were setting it up with the code removal.

[christianrondeau]

@ferventcoder Correct for required, that's what I meant - I typed too fast :)

[christianrondeau]

Here is another (much simpler) pull request. Note that because there was no tests for default options, I didn't write any. I did verify it worked on a build though :)

I kept the two commits with the tests cleanup, just in case: https://github.com/christianrondeau/choco/tree/185-no-prompt-default-failed-pr-1

Let me know if that's better. If you'd prefer that I add tests, let me know.

[ferventcoder]

Always prefer tests :)

On Tuesday, March 24, 2015, Christian Rondeau [email protected]
wrote:

Here is another (much simpler) pull request. Note that because there was
no tests for default options, I didn't write any. I did verify it worked on
a build though :)

I kept the two commits with the tests cleanup, just in case:
https://github.com/christianrondeau/choco/tree/185-no-prompt-default-failed-pr-1

Let me know if that's better. If you'd prefer that I add tests, let me
know.

—
Reply to this email directly or view it on GitHub
#199 (comment).

Rob
"Be passionate in all you do"

http://devlicio.us/blogs/rob_reynolds
http://ferventcoder.com
http://twitter.com/ferventcoder

[christianrondeau]

@ferventcoder I guessed so, I do too 👍

Though in that case I'm not sure how to proceed. Since there was no test verifying that the "default" did indeed work, and since I only removed a feature, not only I have no new tests to write, but I also have no tests to update or remove 😛. The behavior of null default choice is already tested... so, what's your take on this?

[ferventcoder]

I didn't have any tests verifying using the default? Gah, happens when you
are on a deadline - technical debt. :/

On Tuesday, March 24, 2015, Christian Rondeau [email protected]
wrote:

@ferventcoder https://github.com/ferventcoder I guessed so, I do too [image:
👍]

Though in that case I'm not sure how to proceed. Since there was no
test verifying that the "default" did indeed work, and since I only
removed a feature, not only I have no new tests to write, but I also
have no tests to update or remove [image: 😛]. The
behavior of null default choice is already tested... so, what's your take
on this?

—
Reply to this email directly or view it on GitHub
#199 (comment).

Rob
"Be passionate in all you do"

http://devlicio.us/blogs/rob_reynolds
http://ferventcoder.com
http://twitter.com/ferventcoder

[ferventcoder]

👍

[ferventcoder]

I'm +1 on merging this, with possibly fixing the rollback prompt. That's one that seems like it would have a default choice. Thoughts?

[christianrondeau]

My humble opinion would be to avoid defaults altogether. Here is my rationale:

1. If you're asking something, require a response. In the end, you still have to type something, whether it's just Enter or y Enter
2. Personally, I see two choices: rollback (and do something I might not understand/trust), or don't rollback (and let me repair manually). I would probably choose not to rollback, especially when failing an update. Since it's not a clear-cut choice, a default may not make sense.
3. If however rollback should be the de facto choice, then it should not be an option at all.
4. The prompt When prompting for a user yes/no answer, use a short [y/n] representation #181 will shorten the display. I'm not sure how to clearly identify a default option without adding noise to the prompt message.

This being said, those are my only arguments 😄. If you still believe the default value to be preferable, let me know and I'll change the PR to bring back the default option for this specific case.

[ferventcoder]

Merged into stable at e992c4f and will be released in 0.9.9.5.