Skip to content

Added -S flag for truncating long lines - #2309

Merged
sharkdp merged 9 commits into
sharkdp:masterfrom
johnmatthiggins:master
Oct 30, 2022
Merged

Added -S flag for truncating long lines#2309
sharkdp merged 9 commits into
sharkdp:masterfrom
johnmatthiggins:master

Conversation

@johnmatthiggins

@johnmatthiggins johnmatthiggins commented Sep 10, 2022

Copy link
Copy Markdown
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

@Enselic Enselic left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

johnmatthiggins commented Oct 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@johnmatthiggins

johnmatthiggins commented Oct 15, 2022

Copy link
Copy Markdown
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
Comment thread tests/examples/80-columns.txt
@@ -0,0 +1 @@
abcdefghigklmnopqrstuvxyzabcdefghigklmnopqrstuvxyzabcdefghigklmnopqrstuvxyzabcdefghigklmnopqrstuvxyz

@Enselic Enselic Oct 17, 2022

Copy link
Copy Markdown
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.

@johnmatthiggins johnmatthiggins Oct 17, 2022

Copy link
Copy Markdown
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

@Enselic Enselic left a comment

Copy link
Copy Markdown
Collaborator

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.

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

bat()

@Enselic Enselic Oct 17, 2022

Copy link
Copy Markdown
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).

@johnmatthiggins johnmatthiggins Oct 17, 2022

Copy link
Copy Markdown
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.

@johnmatthiggins johnmatthiggins Oct 17, 2022

Copy link
Copy Markdown
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.

@Enselic Enselic Oct 18, 2022

Copy link
Copy Markdown
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

@Enselic Enselic left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread src/bin/bat/clap_app.rs Outdated
@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

@Enselic Enselic left a comment

Copy link
Copy Markdown
Collaborator

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

sharkdp commented Oct 30, 2022

Copy link
Copy Markdown
Owner

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