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

Already on GitHub? Sign in to your account

Add support for ANSI codes #20

Merged
merged 57 commits into from May 9, 2014

Conversation

Projects
None yet
2 participants
Owner

fabiopelosin commented May 8, 2014

Closes #17

Theres is problem with the specs related to the command logic caused by:

  • The command class checking wether the String class has defined the #red, #green and #blue methods to decide wether to enable ANSI output.
  • The help banner chaining according to the ANSI flag being present or not.
  • The random order of the tests which might cause the StringMixin or the StringMixinDisable tests be called before tests depending on the output of the help banner.

I haven't identified a strong solution yet, the possibilities that I see are:

  • Check only if for a TTY.
  • Favor our mixing adding a flag somewhere indicating that the ANSI output has been requested (i.e. provide automatic detection only of the mixin). The drawback of this approach is that clients likely want to load the ANSI code only if needed.

I vote for the first.

/c @alloy

Owner

alloy commented May 9, 2014

Now that we have our own ANSI colours mixin, I’d say that we only need to enable it based on:

  1. Is it a TTY? If so, auto-enable.
  2. Has the user explicitly disabled it? (--no-ansi) If not, auto-enable it if step 1 passes.
Owner

fabiopelosin commented May 9, 2014

To be clear, the question was about the logic to enable the --no-ansi option. I understand to according to your comment 34377be should be good. If not TTY the option is not present. Otherwise its there but enabled by default. The only caveat is that stub mixing is included by default polluting the namespace.

The client of CLAide can still set the property to false and disable the logic.

However now, that I think about it we should just let the client decide and disable any for of auto detection. It is confusing for an user to see an option disappear under certain conditions.

Owner

alloy commented May 9, 2014

I don’t follow your message well enough. What is important is that we have sensible defaults (which is to enable ANSI when possible) and that the end-user can always explicitly enable it.

I.e. when the user is using the CLI from a non-TTY source, then the user should be able to enable it with --ansi.

Owner

alloy commented May 9, 2014

Also, now that we have our own ANSI handling, I think it’s time to get rid of all those methods on String. Helper functions aren’t very friendly either, especially when they have to be used from client code. (E.g. this in CP code would be ugly: CLAide::ANSI.red(string).)

So how about one method on String? E.g. "foo".ansi.red

(Obviously this needs a deprecation period, so the methods should still exist on String for a short period.)

Owner

alloy commented on a13d59d May 9, 2014

Please don’t do this. A Rakefile should be usable without Bundler, because loading it here introduces a chicken and egg problem for rake bootstrap ;)

Owner

fabiopelosin replied May 9, 2014

👍

Owner

alloy replied May 9, 2014

The problem is still there (bundle/gem_tasks), though. You should not require any libs that do not come with Ruby by default and assume they will work, instead these should all be loaded in a begin … rescue LoadError; puts "Disabling tasks, run 'rake bootstrap'"; … end.

Owner

fabiopelosin replied May 9, 2014

The problem is still present with require 'bundler/gem_tasks'. I was thinking about it and I see two routes:

Owner

alloy replied May 9, 2014

The rubocop one is even worse, as it completely assumes Bundler is installed :)

Owner

alloy replied May 9, 2014

The snippet I pasted has demonstrated to make it easy for people to get it up and running, as it guides them through the process. (Unless of course the rubygems-bundler NOEXEC thing is loaded, then all kittens die 😿)

Owner

alloy replied May 9, 2014

Ok, I trust you 😄

❤️

Is there any ruby trickery to avoid having everything wrapped in the begin, rescue?

Alas not. Afaik the only replacement for begin is def, but wrapping everything in a method definition doesn’t seem nicer :-/

I’m ok with not indenting in the begin … rescue block, though, in this specific instance. If that’s what’s bothering you? :)

Owner

fabiopelosin replied May 9, 2014

Ok... I have an idea, wrap only the requires that load tasks in begin and rescue and make all the other requirements inside the tasks (which is good also for performance)... sounds good?

Owner

alloy replied May 9, 2014

Ok... I have an idea, wrap only the requires that load tasks in begin and rescue and make all the other requirements inside the tasks (which is good also for performance)... sounds good?

Hmm, but then the tasks still end up being defined and they will break with ‘arcane’ Ruby exceptions such as LoadError when you try to run them, no?

I think it’s not helpful to the user if rake -T would show tasks that will break at runtime.

Owner

fabiopelosin replied May 9, 2014

What about #22?

Owner

fabiopelosin commented May 9, 2014

I don’t follow your message well enough. What is important is that we have sensible defaults (which is to enable ANSI when possible) and that the end-user can always explicitly enable it.

I.e. when the user is using the CLI from a non-TTY source, then the user should be able to enable it with --ansi.

So basically to my understanding everything is setup correctly to your expectations except this condition which should be removed.

So how about one method on String? E.g. "foo".ansi.red

Clever!

Owner

fabiopelosin commented May 9, 2014

(Obviously this needs a deprecation period, so the methods should still exist on String for a short period.)

I don't think that we need a deprecation period because the clients where using external gems for the colours so they can drop the dependency and switch to our syntax when ready.

Owner

alloy commented May 9, 2014

So basically to my understanding everything is setup correctly to your expectations except this condition which should be removed.

Gotcha, agreed!

I don't think that we need a deprecation period because the clients where using external gems for the colours so they can drop the dependency and switch to our syntax when ready.

Good point!

fabiopelosin added a commit that referenced this pull request May 9, 2014

@fabiopelosin fabiopelosin merged commit 0e78ebe into master May 9, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

@fabiopelosin fabiopelosin deleted the ansi-support branch May 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment