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.

Sign up for GitHub

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

Jump to bottom

Disable wrapping when --plain is used. (Fixes #289) #291

Merged
merged 5 commits into from
Sep 8, 2018
Merged

Disable wrapping when --plain is used. (Fixes #289) #291

merged 5 commits into from
Sep 8, 2018

Conversation

eth-p
Copy link
Collaborator

@eth-p eth-p commented Sep 7, 2018

This is a fix for #289. As I explained in my reply here, it didn't make sense to leave wrapping on with --plain, since the terminal is more than capable of wrapping the lines itself when the sidebar isn't used.

@eth-p eth-p changed the title Wrapping disabled when --plain is used. (Fixes #289) Disable wrapping when --plain is used. (Fixes #289) Sep 7, 2018
@sharkdp
Copy link
Owner

sharkdp commented Sep 7, 2018 •
edited
Loading

@eth-p Thank you very much!

It would be great if we could disable wrapping whenever the style is plain (not just if --plain is passed on the command line, but also if --style=plain is passed).

I also wouldn't care too much about setting --plain and --wrap as conflicting. The auto-mode of wrap would just be never whenever style=="plain". What do you think?

@eth-p
Copy link
Collaborator Author

eth-p commented Sep 7, 2018 •
edited
Loading

@sharkdp Yeah, that sounds like a better (and more consistent) solution.

I removed the disable-wrap functionality from --plain and made it automatic as you suggested.

sharkdp
sharkdp reviewed Sep 7, 2018
View reviewed changes
src/app.rs Outdated
@@ -190,7 +190,7 @@ impl App {
.long("plain")
.conflicts_with("style")
.conflicts_with("number")
.help("Show plain style (alias for '--style=plain').")
.help("Show plain style (alias for '--style=plain'.")
Copy link
Owner

Choose a reason for hiding this comment

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

Just a small leftover missing parens here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! My bad

@sharkdp
Copy link
Owner

sharkdp commented Sep 7, 2018

Thank you for the update. This looks great.

@sharkdp
Copy link
Owner

sharkdp commented Sep 7, 2018

I guess your rustfmt version is slightly outdated. I can run cargo fmt on this branch, if you want.

@eth-p
Copy link
Collaborator Author

eth-p commented Sep 7, 2018 •
edited
Loading

If it's easier for you to do it before merging, sure. I'm definitely going to update mine in the meantime, though.

Edit: Updated my rustfmt to the latest stable and added a commit.
Edit 2: And it still failed the nightly check...

@sharkdp
Copy link
Owner

sharkdp commented Sep 7, 2018

I install rustfmt via

cargo +nightly install rustfmt-nightly -f

@sharkdp sharkdp merged commit 8b4abb0 into sharkdp:master Sep 8, 2018
@sharkdp sharkdp mentioned this pull request Sep 8, 2018
Closed
@eth-p eth-p mentioned this pull request Sep 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharkdp sharkdp sharkdp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eth-p @sharkdp