default value has to be a string

[branch14]

default value has to be a string

[abinoam]

Hi @branch14 ,

I don't plan to patch 1.7-stable branch anymore.
I couldn't reproduce the issue at 2.0 branch.

[abinoam]

@branch14 today we have officially released 2.0.0 version!
I'll close this issue, but feel free to reopen it if you think it's necessary.

Thank you very much for your feedback! 👍

[branch14]

Thank you for your work on highline. I never tried the 2.0 branch, but now that it has been release I totally will. :)

[abinoam]

Thank you! I hope it work well. Give me a feedback as soon as you tested it 👍

[branch14]

I tested it and unfortunately I found the same issue in 2.0.0

Luckily the work around still works. :)

Here is how to reproduce it:

#!/usr/bin/env ruby

require 'highline'
h = HighLine.new

h.ask("Default shows if it is a string: ", String) { |q| q.default = "I am a string" }
h.ask("Default doesn't show if it is an int: ", Integer) { |q| q.default = 42 }
h.ask("But strangely this works: ", Integer) { |q| q.default = "42" }

# % ruby test.rb
# Default shows if it is a string: |I am a string|
# Default doesn't show if it is an int:
# But strangely this works: |42|

[abinoam]

I've reopened the issue.
Could you clone the repo and run rake acceptance and post the results here?

[abinoam]

It was correctly reproduced in my enviroment.

[abinoam]

In a quick look what I see is...

Default value is about the answer gathering part. We should always receive a string by keyboard, although we could have any kind of object as default. This could be useful for several scenarios by passing as default a more meaningful object than nil.

That Integer is more about coercion.

I can see that Highline is doing a type check at this part

    def build_responses(message_source = answer_type)
      append_default if [::String, Symbol].include? default.class

      new_hash = build_responses_new_hash(message_source)
      # Update our internal responses with the new hash
      # generated from the message source
      @internal_responses = @internal_responses.merge(new_hash)
    end

I would prefer to just cast it into a String and check if it is empty or not (to avoid placing two bars side by side without any content inside it. Like ||).

But, any object in Ruby has a string representation by default. And, at that case that I pass bare object I could have the weird result like this:
"Question: |#<Object:0xXXXXXX>| "

From: TestHighLine#test_default_with_non_String_objects [/Users/abinoam/src/highline/test/test_highline.rb:834

So, perhaps sticking with the type checking thing, although not being the more flexible way, solves your problem without any great change at the behaviour of HighLine.

[branch14]

I understand that inputs are always of type string and that the Integer is used for coercion.

I concur that "Question: |#<Object:0xXXXXXX>| " seems weird. But at least it would follow PLOP. Whereas silently ignoring everything except Strings and Symbols violates PLOP.

Personally I would prefer "Garbage in, Garbage out", as it would have been obvuious that I need to pass something with a meaningful string representation.

With the current solution it took me a while to figure out that I had to cast the Integer to a String to make it work.

[abinoam]

I may agree with you. But this breaks the current behaviour. We can discuss this to come in a major release.

For now, I think we can ammend the type checking part adding Integer to the list.
This will solve for your specific case without any major break.
What do you think about this?

[abinoam]

Hi @branch14 what do you think about my last comment 👆

[branch14]

I fully concur. I'm sorry I didn't mean to let you hanging.

…

On Mon, 3 Sep 2018, 17:08 Abinoam P. Marques Jr. ***@***.***> wrote: Hi @branch14 <https://github.com/branch14> what do you think about my last comment 👆 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#233 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABR5H6jK9-3A9jKTTVqK_ymEr4rBl5uks5uXUXqgaJpZM4Ud3vp> .

[abinoam]

Hi @branch14

Sorry for taking so long.
I'll add the requested behavior at the same time adding a switch do disable it.
So people can easily transition between the to behaviors (showing or not showing the Question#default value hint).
Code is ready, opening a PR.

[abinoam]

I'll have to do a major release as this is a breaking change.