Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added index coloring functionality in menu #193

Closed
wants to merge 9 commits into from

Conversation

aregic
Copy link
Contributor

@aregic aregic commented May 25, 2016

A small step toward #100

Also, I have modified .gitignore to ignore vim swap files.

@abinoam
Copy link
Collaborator

abinoam commented May 25, 2016

Hi @aregic,

Thank you very much for the PR!
I'll be reviewing and trying to merge through this week.

@abinoam
Copy link
Collaborator

abinoam commented May 27, 2016

Hi @aregic,

Do you mind if I do some "rebase"?
Your PR is really cool, but I prefer to fix some minor issues to easy my maintenance tasks.
And, I think we shouldn't use colors by default as this will change a lot of the expected behaviour.
Perhaps in a 2.1.0 branch in the future with some more featues added like support for configuration of the color used by code, or by configuration file and etc.
So I think we should drop the default true value for HighLine::Menu.use_color by now.

@abinoam
Copy link
Collaborator

abinoam commented May 27, 2016

Still reviewing your PR, and saw that we already have a HIghLine.use_color with a different semantics. Perhaps one may misunderstand it with HighLine::Menu.use_color. Perhaps we should come up with a different name for it.
Any suggestions?

@abinoam
Copy link
Collaborator

abinoam commented May 27, 2016

Perhaps we should fusion the use_color with index_color like, if index_color == nil then coloring of Menu indexes are disabled. If there's a color on it, we should coloring Menu indexes. What do you think about it?

@abinoam
Copy link
Collaborator

abinoam commented May 27, 2016

And also, what's your e-mail? So I could reset it on the commits.
And I would suggest you to set it on your git configuration.
You may configure it like this

$ git config --global user.name "John Doe"
$ git config --global user.email [email protected]

@aregic
Copy link
Contributor Author

aregic commented May 30, 2016

Do you mind if I do some "rebase"?

Sure, go on. I also don't mind if you use a separate branch for this feature until it is mature enough.

we shouldn't use colors by default as this will change a lot of the expected behavior.

One hand, you should have as many features enables as possible, since users will just try it on default before deciding if it's worth looking into or not.
On the other hand, this feature is definitely not mature enough yet.

I leave this decision to you.

Perhaps in a 2.1.0 branch in the future with some more features added like support for configuration of the color used by code, or by configuration file and etc.

Already working on it, will be in my next PR. To be honest the point of handing in such small change was to have feedback soon and to see if this project is even alive :)

Still reviewing your PR, and saw that we already have a HIghLine.use_color with a different semantics. Perhaps one may misunderstand it with HighLine::Menu.use_color. Perhaps we should come up with a different name for it.

I chose this name intentionally, since they do the same thing only in different classes. Nevertheless, the relation should be more clear then it is now: hard to figure out if HighLine.use_color overwrites the HighLine::Menu.use_color (currently it doesn't) or if it should. I might have an idea, see next answer.

Perhaps we should fusion the use_color with index_color like, if index_color == nil then coloring of Menu indexes are disabled. If there's a color on it, we should coloring Menu indexes. What do you think about it?

I could add a :no_decoration or something similarly named symbol to the possible predefined styles which would add nothing to the text. It would be very expressive. I will start on this path, if you don't mind.

about the email: well I just forgot to set it to something meaningful at first, but generally I just don't set it in fear of spam. I have made it public on my profile now.

@abinoam
Copy link
Collaborator

abinoam commented May 31, 2016

About the "on by default" argument, this gem is more than 10 years old, we could say that it has a "stable" code, so for this 2.0.0 release we're working on we have already broken some of the api but we do not want to introduce radical changes to the expected behaviour. This would break a lot of tests on others gems that depend on HighLine. So we prefer to be cautious. But I completely understand your point.

Good to know you're working on improving the feature.

We strongly favor small PRs.

I have already "fixed" the "same method name" problem in my rebased branch, have a look a it.
No problem if you improve it later.

Now that you have a public e-mail your contributions will show up at your GitHub page.
I have rebased the branch again to change the author of every commit to your e-mail, so GitHub may know that you are the author of those commits.

@abinoam
Copy link
Collaborator

abinoam commented May 31, 2016

Closing this because it was merged in rebased branch at PR #194

@abinoam abinoam closed this May 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants