Skip to content

Support relative negative line ranges #3068

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 6 commits into from
Apr 15, 2025

Conversation

ajesipow
Copy link
Contributor

@ajesipow ajesipow commented Aug 10, 2024

Adds support for relative negative line ranges, e.g. -10: or :-10, see #2944.

This is implemented by buffering as many lines as the largest negative offset as a "look ahead" to determine if a line should be printed.

For example:

  • for a range -10: 10 lines are buffered and only once EOF is reached will the (last) lines in the buffer be emitted
  • for a range :-10 10 lines are buffered and lines are emitted as the buffer moves through the file, once EOF is reached the last lines in the buffer are discarded


#[derive(Debug, Clone)]
#[derive(Debug, Copy, Clone)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may not actually want to add this, since LineRange is pub and a future impl could make it not Copy and therefore require a breaking change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO we can deal with that once and if we're in that situation

Copy link
Collaborator

@keith-hall keith-hall left a comment

Choose a reason for hiding this comment

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

Thanks for your hard work on this, and your patience. I think it looks good :) I will ask another maintainer to review before merging though 👍

@keith-hall keith-hall requested a review from Enselic April 14, 2025 17:42
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.

I haven't looked closely enough to Approve, but I have no objections to merging this.

The most important part as far as I'm concerned is the presence of integration/regression tests, which is present here.


#[derive(Debug, Clone)]
#[derive(Debug, Copy, Clone)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO we can deal with that once and if we're in that situation

@keith-hall keith-hall enabled auto-merge April 15, 2025 17:27
@keith-hall keith-hall merged commit 2ff2a81 into sharkdp:master Apr 15, 2025
23 checks passed
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