Skip to content

Consolidating the logic for printing output #2353

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

Merged
merged 1 commit into from
Jan 11, 2018

Conversation

davidalber
Copy link
Contributor

This is my followup to this comment from #2292:

After this PR, I'm going to spin up a new one to merge print_diff_fancy and print_diff_basic by adding more implementation to the enum. I think the enum is too abstract like this.

I actually eliminated the PrintType enum and created a struct -- OutputWriter -- that contains the logic for determining whether to write output using println! or a Terminal instance from the term crate. This allowed print_diff_fancy and print_diff_basic to be consolidated into print_diff.

Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! I left an inline comment on minor refactoring. Other than that the code LGTM.

pub fn writeln(&mut self, msg: &str, color: Option<term::color::Color>) {
match &mut self.terminal {
Some(ref mut t) => {
if color.is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can write if let Some(color) = color { rather than calling unwrap() below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😊 Thanks for catching that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it was such a small change, I rewrote history instead of adding a new commit so you can see it easily. You can see the change here.

Thanks again!

@topecongiro topecongiro merged commit 41b14b6 into rust-lang:master Jan 11, 2018
@topecongiro
Copy link
Contributor

Thank you for the update!

@davidalber davidalber deleted the merge-print-diff branch January 14, 2018 02:36
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.

2 participants