Eliminated colons and added single quotes to list of valid menu choices

[keithrbennett]

I first started using this gem today. When I tried entering 'x' in the examples/menus.rb example in the last test, I got a list like:

:load, :save, :quit, :help

Not knowing any better, I tried entering one of those choices literally, including the colon. That didn't work, but typing it without the colon did.

Omitting those colons in the list viewed by the user would make a right choice more intuitive. I also added single quotes, but you may think that excessive, and feel free to remove them. It now looks like this:

'load', 'save', 'quit', 'help'

[keithrbennett]

I see now that the first map(&:to_s) is redundant since #{s} will implicitly call to_s. I guess the explicitness of the current approach isn't a bad thing though. Feel free to remove it if you like.

[JEG2]

I'm certainly open to making this more clear. If we're going to drop the colon though, adding in single quotes seems to take us back into confusing territory. Is there another way we could clarify these messages? I say messages, because we should probably also fix the ambiguous message above the one you improved, don't you think?

[keithrbennett]

We can certainly omit the single quotes. What do you mean by another way? To make it still clearer? Or structurally?

Regarding the other occurrences of the message, I did not know how to trigger the behavior to test them, so I wasn't confident that modifying them would work correctly. I proposed this pull request as something fast and simple that could offer a little improvement.

Just now I modified the method to use the same string building (minus the single quotes) in a couple of other cases too (see https://gist.github.com/keithrbennett/8222098). I could do something similar in question.rb. However, unfortunately, I don't have time to study the code and test it thoroughly with confidence. If you want to forget about this, I'll understand; I realize it's probably not that important in the grand scheme of things.

[JEG2]

What if we showed a message like this?

You must choose one of the following: load, save, quit, or help.

[keithrbennett]

Ok, I've just changed "Please choose" to "You must choose". In the build_responses method in both Question and Menu, I create a string with the message, and use it in the cases of :ambiguous_completion and :no_completion. I (manually) tested it using the menu example, and I think it does exactly what you want. Should I make a new pull request?

[keithrbennett]

By the way, I'd love to consolidate the 2 methods, but, again, wouldn't be confident that I'd have everything covered. It looks like the only things that vary in the 2 methods are:

1) the object from which to build the responses (options vs. answer_type) 2) whether or not merge is called in the :not_valid case.

We could easily parameterize them, perhaps with default values for the base class, where the derived class just calls the base class method, overriding the defaults:

# In the Question class:
def build_responses(message_source = answer_type, merge = true)
  # ...
end

# In the Menu class:
def build_responses
  super.build_responses(options, false)
end

I'd be happy to write the code, but may not catch any errors, and don't have the time it would take to do this as thoroughly as it deserves.

[JEG2]

In the code above, I believe you meant:

# In the Menu class:
def build_responses
  super(options, false)
end

By all means, please feel free to prepare a patch.

[keithrbennett]

Thanks for the correction.

Ok, a patch rather than a pull request? Should I post it as a gist?

[JEG2]

A pull request is fine. Sorry. It was a poor choice of words on my part.

[keithrbennett]

James, I thought more about this, and studied the code, and realized that the merge happens either way, it's just the direction that changes. This change worked with the menu example, but I haven't tested it further.

[JEG2]

Hmm, I took a look at this in preparation for merging it and I noticed there's a warning related to this change. I don't personally remember why this was done. Any chance you do @sandal?

[practicingruby]

I don't know why it was done, but it does look like the code in Menu has a similar structure. It's been forever since I've touched HighLine so I'm unsure how well our test coverage would catch this. Maybe add a test if necessary and then go from there?

[JEG2]

I knew it was a long shot, but just thought I would check. Thanks anyway.

I'll dig deeper into this patch when I have some more time…

[keithrbennett]

James, Gregory -

I'm pretty sure the warning is just there to alert the developer that there is a strong connection between the 2 methods, and, for example, if you change a key name in one, you'd better change it in the other. They are, after all, modifying the same variable and one is supposed to be able to override the other. Also, that a change in message format in one should be accompanied by the same or equivalent change in the other, for UI consistency.

That's why I deleted the comment -- since the duplication was eliminated, I'm pretty sure it no longer applies.

[JEG2]

I'm more interested in why we felt the need to add the comment. There's plenty of crufty code in HighLine, but I do think we knew how to call an inherited method when we built the menus. It looks like we decided not to do that and added a warning about what we did do. I'm worried we had a good reason for not merging them that I've simply forgot about. Does that make sense?

[keithrbennett]

Based on my own experience, I was imagining that when that code was written, I would see the duplication, try to fix it, run into a problem, and then, for expediency's sake, leave the duplication in, but document it.

That said, you would certainly know better than I. And, it's true that I have not tested it thoroughly.

So let me know what you would like me to do. I can propose the original minimal fix, or nothing at all, or feel free to recommend.

[JEG2]

Can you add a spec or two, just to make sure the old and new behaviors is consistent? With that, I'll merge this request. We can always change our mind if it leads to other problems down the road.

[keithrbennett]

James -

How about if I just get the existing tests to pass? :) I'm embarrassed to say that although I was running the example to see if it worked, I did not run the tests. When I did, I realized that I wasn't finished. I was surprised to see that the message_source could be a lambda, and it looks like .inspect was being called on it for inclusion into the error message -- but I'm pretty sure that was happening before. In any case, I did verify that the existing tests passed (after modifying them not to expect the colons).

[JEG2]

Thanks.

[keithrbennett]

Thanks to you, James!