Fix bugs and add text override options to menu choices

[matrinox]

• Add text: key for menu choices so you can have a string that the user can match to but also a very long text that explains the selection better
• Change choice interface to using keyword args. help is now help: instead of the 2nd arg
• Fix bug where autocomplete prompt does not include menu choices after the first
• Add specs to cover the new features and the fixed bug

[abinoam]

Hi @matrinox,

First of all thank you very much for contributing code!

Some tests are failing, could you have a look at them? I'll have to have a deeper look at the PR so I can be sure it's aligned with the remaining code. But unfortunately it's monday and I'm a little busy today, I'll try to do it throughout the week, any problem?

With a fast look at the code, I see you are changing the API, well, we try to avoid as much as possible such "disruptive" changings, we favor "additive" ones. Sorry if I'm asking you too much, but could you try to "add" the new behaviour preserving the old one? If I have time this week, I can try to assist you.

But, if this is not possible, and the new behaviour is better than the old one, we have place for it as we are preparing a major release (2.0.0) that has already broken some of the old api.

[abinoam]

Are you aware of PR #182 of @tukanos? It's (side) related to your issue. Basic add more ways to customize HighLine to different needs, and this is good!

[matrinox]

I'll incorporate your feedback soon. I tried to avoid as many API changes, too, but I felt that the keyword args is just better long term before it becomes 4 or 5 positional args. Either way, for now I'll figure out a way to make it work with existing API and revert some of the tests.

[matrinox]

The require 'pry' was for some quick testing. Didn't mean for them to be committed, usually I'm on top of those things. I made some changes that hopefully should satisfy your requests. Hope you like it, I enjoy using this library for my own use.

[abinoam]

Sorry for late answer. Didn't have the time yet. I'll try next week.

Em Ter, 26 de jan de 2016 02:35, matrinox notifications@github.com escreveu:

The require 'pry' was for some quick testing. Didn't mean for them to be committed, usually I'm on top of those things. I made some changes that hopefully should satisfy your requests. Hope you like it, I enjoy using this library for my own use.

— Reply to this email directly or view it on GitHub https://github.com/JEG2/highline/pull/184#issuecomment-174841471.

[abinoam]

Hi @matrinox ,

I think the PR is ready to be merged. Could you update the Changelog.md and the lib/highline/version.rb so they reflect the changes introduced by your PR? Then we are ready to merge.

Thank you very much for your contribution! :+1:

[abinoam]

Some more questions. Why have you added the Gemfile.lock? I think it is unecessary and may introduce some problems, but feel free to disagree! If you agree, could you remove the commit that adds it? (You can git rebase if you want).

Is this "m" gem really necessary? We will try to maintain compatibility with Rubinius, JRuby and Windows also. So, the less gems we depend on, the better. Again, we have no problem if you disagree and we can discuss the issue if necessary. Make yourself at home!

[abinoam]

For the Changelog.md follow this model

PR #178 - Improve score on Code Climate by applying some refactoring. By Abinoam Jr. (@abinoam)

[matrinox]

Hi @abinoam, I've been busy recently but I'll respond to your comments as soon as possible and follow the conventions. I'm not for or against Gemfile.lock, it's something that got added when I did git add -A probably. I'll remove the file add that to .gitignore, if that's alright with you.

The "m" gem is also not necessary, it's just way easier for me to test only 1 at a time so I'd thought I'd share that.

Can you clarify one thing for me:

> I personally don't like this pattern of branching on type test.

What do you mean by branching on type test? Let me know and I can make the necessary changes.

Nevermind, I forget that these comments actually link to code. Unfortunately there aren't that many good options without breaking API. What do you think about this:

def choice( name, help = nil, text = nil, &action )
  item = MenuItem.new(name: name, text: text, help: help, action: action)
end

This keeps backwards compatibility and then we can improve on the interface in 2.0.

[abinoam]

You have an "if" that tests the type of the received object and then have 2 branches of behaviour based on type. I used the word "pattern" in the general sense. I don't remember if this pattern have a proper "name" for it. In this case it's a little hard to get rid of it as we're trying to maintain both behaviours, the old and the new one.

Don't rush. Take your time.

PS: I'm not good at english, then sometimes it's hard for me to express correctly. So if you see me saying something strange, ask.

Em Seg, 1 de fev de 2016 04:09, matrinox notifications@github.com escreveu:

I've been busy recently but I'll respond to your comments as soon as possible and follow the conventions. I'm not for or against Gemfile.lock, it's something that got added when I did git add -A probably. I'll remove the file add that to .gitignore, if that's alright with you.

The "m" gem is also not necessary, it's just way easier for me to test only 1 at a time so I'd thought I'd share that.

Can you clarify one thing for me:

I personally don't like this pattern of branching on type test.

What do you mean by branching on type test? Let me know and I can make the necessary changes.

— Reply to this email directly or view it on GitHub https://github.com/JEG2/highline/pull/184#issuecomment-177817265.

[matrinox]

@abinoam ready for review

[abinoam]

Thank you very much @matrinox, for your patience in trying to address my comments/suggestions. It's a really good PR for HighLine. I'm ready to merge and release it. Hooking @JEG2 here if he wants to give some input on it.

Just one more question, do you like git rebase? I see this sequence of commits where something is introduced and then removed a good oportunity for a rebase before merging into master. But if you're not confortable with rebase, just let the way it is.

[abinoam]

@matrinox I have already rebased it on #186

Let's merge it from there.

[abinoam]

Hi @matrinox ,

We should look at this PR more carefully. Reopening it.

Look at the example of the #choice interface.

# @example Use of help string on menu items

cli.choose do |menu|
  menu.shell = true

  menu.choice(:load, "Load a file.")
  menu.choice(:save, "Save data in file.")
  menu.choice(:quit, "Exit program.")
  menu.choice(:load, text: 'MUDEI o text', help: "MUDEI o help")
  menu.choice(:save, help: "Save data in file.")
  menu.choice(:quit, help: "Exit program.")

  menu.help("rules", "The rules of this system are as follows...")
 end

I think it's not working like this.

I will not have any more free time today. But I promise to give priority to this PR through the week as you have made such an effort. Thank you again, and sorry for being so exigent/rigorous/demanding.

[matrinox]

I'll fix the remaining stuff in the other PR tonight if I can

[matrinox]

Don't worry, @abinoam, you're not being demanding. Your project, your rules, I'm happy to help

[JEG2]

In general, I'm a fan of what I'm seeing here.

I worry a little bit that we're forcing @matrinox to backtrack on his ideas a lot to accommodate the existing interface. If that's the case, perhaps we should just compare the existing and recommended approaches. If we like the new one better, we could just switch over in the 2.0 development branch. Just a thought.

[matrinox]

@JEG2 I'm happy to discuss 2.0 features and anything I could help with.

On a side-note, what documentation system are you planning on using? Also, any preference on enforcing styling, such as with rubocop?

[JEG2]

On a side-note, what documentation system are you planning on using?

We are currently publishing YARD to RubyDoc.info. @abinoam set that up.

Also, any preference on enforcing styling, such as with rubocop?

Hmm, I'm not sure if I wish to go that far. We can if others feel strongly about it, but I disagree with some of the points in the style guide it's based off of and would probably choose to customize it a bit.

[abinoam]

If you're using rubocop I would say the first tweaking is

StringLiterals:
  Enabled: false

I use it sometimes just as a hint.

I'm in a really busy week. Most of my time is being off the computer. So sorry for late answers. @matrinox feel free to defend your changes. I'll take a look at them more carefully this weekend. And, as @JEG2 said, perhaps we should just jump into the new api.

[abinoam]

Closing this as it was merged from #186