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

Add support for BusyBox less as pager #2162

Merged
merged 4 commits into from
May 4, 2022
Merged

Add support for BusyBox less as pager #2162

merged 4 commits into from
May 4, 2022

Conversation

nfisher1226
Copy link
Contributor

See issue #2143

This is an initial working draft. I haven't updated the CHANGELOG, but will before merge if accepted.

@nfisher1226 nfisher1226 marked this pull request as draft April 22, 2022 14:19
@keith-hall
Copy link
Collaborator

Looks good to me, but my Rust code reviews are lackluster compared to those of our other maintainers, so let's see what they say :)

I think to fix CI after unmarking the PR as a draft, you just need to execute tests/syntax-tests/update.sh and commit the file which will change :)

@nfisher1226 nfisher1226 marked this pull request as ready for review April 22, 2022 23:00
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.

Thanks a lot for working on this! This is a very good start. I added some comments. For the record, this PR does not change the public API of bat-as-a-library, so we don't need to be super picky about naming of things:

% pr=2162; git fetch -f upstream pull/$pr/head:$pr && git checkout $pr
% cargo install cargo-public-api
% cargo public-api --diff-git-checkouts origin/master 2162                
Removed items from the public API
=================================
(none)

Changed items in the public API
===============================
(none)

Added items to the public API
=============================
(none)

Also add an entry to CHANGELOG.md please 🙏 Edit: I see now that you already mentioned you would do that.

src/less.rs Show resolved Hide resolved
src/less.rs Outdated Show resolved Hide resolved
src/output.rs Show resolved Hide resolved
src/output.rs Show resolved Hide resolved
tests/syntax-tests/highlighted/Rust/output.rs Outdated Show resolved Hide resolved
src/output.rs Show resolved Hide resolved
* Revert all changes in `test` directory
* Minimize overall diff size
* Detect busybox from separate helper function
* Pass equivalent options to BusyBox from same code by changing from long to
  short options
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 diff was much smaller and easier to review now. Thanks! I have 3 more comments.

src/less.rs Show resolved Hide resolved
src/output.rs Outdated Show resolved Hide resolved
src/output.rs Outdated Show resolved Hide resolved
Add test for invalid utf-8
Add `parse_less_version_busybox` to test for invalid program
Add commenting around short options
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.

This comes across to me as a low-risk diff now that should solve the issue. Thanks a lot for the help.

@nfisher1226
Copy link
Contributor Author

Many thanks for working with me on this. It was a good learning experience.

@Enselic
Copy link
Collaborator

Enselic commented May 4, 2022

Let's go ahead and merge this now since both me and Keith has Approved

@Enselic Enselic merged commit 5114c01 into sharkdp:master May 4, 2022
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.

3 participants