piotrmurach / tty-command

Execute shell commands with pretty output logging and capture stdout, stderr and exit status.
https://ttytoolkit.org
MIT License
400 stars 34 forks source link

Add option to separate `pretty` command output with new line #47

Closed thisismydesign closed 6 years ago

thisismydesign commented 6 years ago

I found this style of output to be more readable.

screen shot 2018-04-24 at 17 50 33

I made an implementation hijacking your printer classes but that wasn't a smart thing to do (https://github.com/thisismydesign/autowow/issues/13). As an alternative I'm wondering whether you would accept such an option?

This is just a quick draft, todo:

piotrmurach commented 6 years ago

I'm not really in favour of such option. Why is that? Partly it is a matter of taste in designing libraries, I want the code to be flexible with the minimum api. I don't really want to have multiple options that 'configure' printers in one way or the other. Not only this option applies to one formatter, more importantly, there is a way for yourself to create a custom formatter that overrides this particular behaviour.

thisismydesign commented 6 years ago

Yeah that's also what I had in mind as a concern.

What I would say though is that the built-in printers are important part of your gem with ever increasing functionality (pretty would be a good example). Copy-pasting the functionality therefore doesn't make much sense to me and the hijacking approach I did isn't great either because it might break (as it did).

Another alternative would be to make the default printers part of the public API (essentially consider breaking changes there when making version increments). This would allow others to safely extend the printers with custom behaviour. Would you be open to that? (To be fair the gem according to version 0.* is still in beta, so technically breaking changes are ok and it might already be expected that others extend your printer classes?)

piotrmurach commented 6 years ago

Firstly, my advice would be to change how you manage dependencies. Specifying the following:

spec.add_dependency "tty-command", "<= 0.8.0"

is something I've hardly ever come accross if ever. Usually people specify lower boundary. You're essentially saying that 0.1.0 is fine, hence you have code detecting gem versions. I'm following semantic versoning, where patches within minor version do not break. So in your case I would encourage your to use:

spec.add_dependency "tty-command", "~> 0.8.0"

which would allow you to remove all the version specific code and bullet proof you for the future.

Secondly, I can only appologise for breaking your code. However, the current version of the abstract printer is much more flexible and I don't expect it to change any time soon. Therefore, I would like you to treat printers as having public api, which shouldn't break. Having, the suggested dependency restrictions you will ensure that your code doesn't break randomly.

thisismydesign commented 6 years ago

Sure you're right about the dependency handling, thanks for the advice. I was messing around with versions because of the other discussion we're having in parallel. :) So will probably have to keep some of the version specific code, but I should definitely set a lower limit.

No apologies needed, thanks for all the support.