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

No color options, print support for windows #18

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

No color options, print support for windows #18

wants to merge 5 commits into from

Conversation

stegmanh
Copy link

@stegmanh stegmanh commented Apr 7, 2017

This PR was made to address issues in -> #15

First rust contribution so I'll preface to say if things seem messed up/wrong let me know and I'll try my best to fix them!

  • Added support --word-diff flag. If that flag is set, then instead of colors, text will be used to distinguish diffs in the text.
  • Added a new 'constructor' that takes in an options object. (Just word-diff for now, but eventually this can hold a bunch of options)
  • Added options struct that will hold options that difference.rs might use later
  • Un deprecated print_diff. This was done because AFAIK the term module only supports stdin and stdout. So to leverage this library to support windows, I took the avenue to have a function that will print directly to stdin and stdout so other terminals are supported.

@stegmanh
Copy link
Author

stegmanh commented Apr 7, 2017

Appears travis is failing on nightly due to a dep or sub dep not being able to compile?

@johannhof johannhof self-requested a review April 8, 2017 05:54
@johannhof
Copy link
Owner

Hi there, thanks for the contribution. Travis is failing because Clippy seems to be borked on the latest Nightly again, nothing to worry about. If it compiles again and we find that we missed some lint, we'll just push the fix afterwards.

This looks good from a quick glance, but I'll get to reviewing later today.

@colin-kiegel
Copy link
Collaborator

any updates here? Is it possible to restart the CI build?

@johannhof
Copy link
Owner

Hey, thanks again for the PR. Sorry for taking so long to get back to you.

I have two concerns with this patch. I don't think we should introduce term as a dependency and I would like to avoid un-deprecating print_diff. IMO the library should focus on computing the difference and not pretty-printing the results.

Maybe we should just make it really simple and do word_diff if cfg!(windows).

@colin-kiegel
Copy link
Collaborator

Hm, I can understand the concern the concern about additional dependencies.

Whithout knowing the details of this PR .. what if the additional dependency was behind some kind of feature flag?

@johannhof
Copy link
Owner

I'm not sure why consumers of this library can't just depend on term instead and use one of the formatting examples I provided. It shouldn't be a big effort, the formatting code isn't that large and it gives consumers much more flexibility.

@upsuper
Copy link

upsuper commented Jul 24, 2017

IMO the library should focus on computing the difference and not pretty-printing the results.

In that case, this crate should simply print +/- without any color, so that it produces reliably and reasonable result across all platforms.

Having it produce silly result on Windows by default, or it produce different result based on platforms would hurt consumers of this crate in different ways.

@johannhof
Copy link
Owner

Having it produce silly result on Windows by default, or it produce different result based on platforms would hurt consumers of this crate in different ways.

I can see that there are edge cases where you'd want the print output to be exactly the same across platforms, but would this really be such a big pain on consumers to implement their on printing code if they have such requirements? Maybe I'm just underestimating the use case for having consistent output on all platforms.

In that case, this crate should simply print +/- without any color, so that it produces reliably and reasonable result across all platforms.

Considering that colorful output (only for some consumers unfortunately) has been the default for some time, would you consider an optional nocolor compiler flag, that makes the behavior consistent, an acceptable solution?

@upsuper
Copy link

upsuper commented Jul 24, 2017

I can see that there are edge cases where you'd want the print output to be exactly the same across platforms, but would this really be such a big pain on consumers to implement their on printing code if they have such requirements? Maybe I'm just underestimating the use case for having consistent output on all platforms.

It may not be a big pain to implement that, but it may give downstream tools and their users false expectation if they don't check behavior on different platform carefully.

For example, a developer on Windows may think this crate would reliably produce text-based diff, so that users can use commandline tools to consume its output, however it is not true on other platforms. This can happen vice versa. It would just make people confusing.

I think Display should have reliable behavior. If you think this should just be a best-effort thing, this should probably be done in Debug instead.

Considering that colorful output (only for some consumers unfortunately) has been the default for some time, would you consider an optional nocolor compiler flag, that makes the behavior consistent, an acceptable solution?

A reliable behavior should be the default, I think. Wouldn't it be fine to just implement the coloring in the commandline tool, but leave the lib part generate text result?

Alternatively, you can probably add an optional default feature with term dependency to generate reliable colored result on all platforms, so that it keeps the original default behavior while still allows users to remove that dependency.

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.

4 participants