Option to bypass i18n question prefix.

[simon-soak]

I've found the i18n "question" format doesn't work when I'm not requesting a user input. Say I want to step through the shared folder and ask the user whether or not it should be uploaded, you might have:

set :ask_upload, ask("Do you wish to upload folder x", 'y')
$ask = fetch(:ask_upload)
if ($ask == 'y') ...

Which gives you Please enter Do you wish to upload folder x (y):

For a better format and more flexibility with the question text (essentially getting rid of the prefix "Please enter " without modifying i18n.rb). You can now use;
ask(:ask_upload, 'y', {text: "Do you want to upload x (y): "}
if (fetch(:ask_upload) == 'y') ...

I'm hope you understand what I'm trying to achieve, I did think about modify the i18n file but the impact would be too huge.

[leehambley]

In principal I think I could live with this change.

Please have a look at the tests and ensure this behaviour is tested, and also review the contribution guidelines and see how to make this into a PR that we can accept, and we'd be more than glad to merge it.

[mattbrictson]

Hi @simon-soak, this a good improvement. Thanks for the PR.

I want to suggest a few changes to your implementation:

• Your test? method serves dual purpose as both a predicate and as an accessor. I think this is a little confusing.
• Also, test? is never used outside the question method, so I think it is an unnecessary abstraction.
• I assume that both nil and false values for the :text option should result in the default i18n behavior. If that's the case, then fetch(:text, false) is redundant and you can just use [:text].

How about this:

def question
  custom_text = (options || {})[:text]
  return custom_text if custom_text
  I18n.t(:question, key: key, default_value: default, scope: :capistrano)
end

Further, I think the (options || {}) check repeated in this class is a code smell. Maybe you can change the initialize method to fix the nil problem there instead? Then we won't need those repeated checks.

def initialize(key, default, options={})
  # ...
  @options = options || {}
end

Finally, I would appreciate it if you could also:

1. Add a test for this new behavior to question_spec.rb.
2. Update CHANGELOG.md with a bullet point describing this new feature.

Thanks!

[simon-soak]

Thank you both for the feedback. I've actioned your points and amended my initial commit.

Ruby is not my principle language, so I have absolutely no idea where to start with question_spec.rb tests. If you could advise or take off my hands that'll be grand.

[mattbrictson]

My RSpec is also a bit rusty, but I think this will be a good test:

--- a/spec/lib/capistrano/configuration/question_spec.rb
+++ b/spec/lib/capistrano/configuration/question_spec.rb
@@ -15,6 +15,28 @@ module Capistrano
         end
       end

+      describe "prompt" do
+        before do
+          $stdin.expects(:gets).returns("")
+        end
+
+        context "without :text option" do
+          it "uses i18n" do
+            $stdout.expects(:print).with("Please enter branch (default): ")
+            question.call
+          end
+        end
+
+        context "with :text option" do
+          let(:options) { { text: "Custom prompt: " } }
+
+          it "uses :text value" do
+            $stdout.expects(:print).with("Custom prompt: ")
+            question.call
+          end
+        end
+      end
+
       describe "#call" do
         context "value is entered" do
           let(:branch) { "branch" }

[mattbrictson]

@simon-soak If you're still interested in finishing this PR, and you need further assistance, let me know!

[simon-soak]

Morning @mattbrictson, apologies for the hold up.

I've managed to get some more time on this, and implemented (copy/pasted) the rubocop code into question_spec.

However; on running of rake rubocop it's thrown up some offenses "no-nested-conditiontals" on;

• features/support/vagrant_helpers.rb:28:5:
• lib/capistrano/configuration/validated_variables.rb:94:9:
• lib/capistrano/version_validator.rb:8:7:

I'm pretty sure I didn't get this last time, so I imagine rubocop has been updated, anythoughts?

[leehambley]

@simon-soak yeah, Rubocop is a moving target, you can safely ignore any errors that were there before your changes based on rubocop HEAD, and we'll eventually clean them up

[mattbrictson]

@simon-soak RuboCop is working in master, so if you could rebase your PR against master and resolve any conflicts, that should clear up those RuboCop offenses and allow us to merge this PR. Thanks again!

If you'd like me to handle the rebase, let me know.

[simon-soak]

Awesome, I should be getting more time on this this Friday or next week.

Feel free to do your thing, if you'd rather, I've ticked the "Allow edits from..." box.

[will-in-wi]

@simon-soak: Are you able to rebase this?

[will-in-wi]

I just pushed a rebase. Is there still interest?