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

Added -S flag for truncating long lines #2309

Merged
merged 9 commits into from
Oct 30, 2022
Merged

Added -S flag for truncating long lines #2309

merged 9 commits into from
Oct 30, 2022

Conversation

johnmatthiggins
Copy link
Contributor

Implementation of feature mentioned in issue #2239. Added both --chop-long-lines and -S as flags.

@johnmatthiggins johnmatthiggins mentioned this pull request Sep 11, 2022
Closed
Enselic
Enselic requested changes Oct 1, 2022
View reviewed changes
Copy link
Collaborator

@Enselic Enselic left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! Please also add one or two basic regression tests for this change. And also add an entry to CHANGELOG.md please.

@Enselic Enselic added the waiting-on-author Progress on this PR is blocked mostly because we are waiting on the author of the PR to do something label Oct 9, 2022
@johnmatthiggins
Copy link
Contributor Author

johnmatthiggins commented Oct 10, 2022 •
edited
Loading

I added some tests and added the change to the CHANGELOG.md file. It is now ready for review.

@johnmatthiggins
Copy link
Contributor Author

Oops it looks like I had reversed logic and set -S and --chop-long-lines as aliases for wrap=character instead of wrap=never. This has now been remedied. Also for the sake of clarity, I added four tests to the project and I'll explain each of them.

  • The first test verifies that no line wrapping happens when the wrap=never command line flag is present.
  • The second test verifies that line wrapping happens when the wrap=auto command line flag is present.
  • The third and fourth tests verify that no line wrapping happens when -S or --chop-long-lines is specified.

I added some tests that cover historical functionality not implemented by this PR because those cases were not yet covered and I wanted to make sure that my changes didn't alter existing functionality.

Enselic
Enselic reviewed Oct 17, 2022
View reviewed changes
tests/examples/80-columns.txt Outdated
@@ -0,0 +1 @@
abcdefghigklmnopqrstuvxyzabcdefghigklmnopqrstuvxyzabcdefghigklmnopqrstuvxyzabcdefghigklmnopqrstuvxyz
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this file be called 100-columns.txt? Another proposal: long-single-line.txt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I like long-single-line.txt, I'll go with that.

Enselic
Enselic reviewed Oct 17, 2022
View reviewed changes
Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of nit-picks.

tests/integration_tests.rs Outdated
let expected =
"abcdefghigklmnopqrstuvxyzabcdefghigklmnopqrstuvxyzabcdefghigklmnopqrstuvxyzabcdefghigklmnopqrstuvxyz\n";

bat()
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a substantial amount of copy-paste here. Would be nice to introduce a helper function that takes as parameter the only thing that varies among the tests (expected output and the wrapping arg).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a helper function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the helper function in the tests/integration_tests.rs file, but is this the correct procedure? I don't see any other helper functions in this file, so I figured I should ask.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep it was correct to put the helper next to the functions that need it

I don't expect any other code to ever need that helper, so it makes sense to put it where you put it

@Enselic Enselic removed the waiting-on-author Progress on this PR is blocked mostly because we are waiting on the author of the PR to do something label Oct 17, 2022
Enselic
Enselic reviewed Oct 18, 2022
View reviewed changes
Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

The test code looks much better now, thanks! I found one more comment to make

src/bin/bat/clap_app.rs Outdated Show resolved Hide resolved
@johnmatthiggins @Enselic
Update src/bin/bat/clap_app.rs for PR 2309
ff71303 
Co-authored-by: Martin Nordholts <enselic@gmail.com>
Enselic
Enselic approved these changes Oct 20, 2022
View reviewed changes
Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

Looks mergeable to me, thanks!

@sharkdp sharkdp merged commit 3c9c960 into sharkdp:master Oct 30, 2022
@sharkdp
Copy link
Owner

sharkdp commented Oct 30, 2022

Thank you very much

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

@Enselic Enselic Enselic 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.
3 participants
@johnmatthiggins @sharkdp @Enselic