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

Correctly render tab stops in --show-all #2038

Merged
merged 6 commits into from
Mar 6, 2022

Conversation

Synthetica9
Copy link
Contributor

@Synthetica9 Synthetica9 commented Jan 20, 2022

New behaviour:

───────┬────────────────────────────────────────────────────────────
       │ STDIN
───────┼────────────────────────────────────────────────────────────
   1   │ ··├┤␊
   2   │ ·├─┤␊
   3   │ ├──┤␊
   4   │ 12345678␊
───────┴────────────────────────────────────────────────────────────

Old behaviour

───────┬────────────────────────────────────────────────────────────
       │ STDIN
───────┼────────────────────────────────────────────────────────────
   1   │ ··├──┤␊
   2   │ ·├──┤␊
   3   │ ├──┤␊
   4   │ 12345678␊
───────┴────────────────────────────────────────────────────────────

Does not work with multi-width characters, yet, is this a requirement? If so, how should this work with unicode escaped characters?

@Enselic
Copy link
Collaborator

Enselic commented Jan 22, 2022

Nice 👍 I skimmed the code and it looks reasonable. Before a detailed review, can you please add at least one test (preferably more as long as it makes sense, e.g. with different tab sizes) for this in https://github.com/sharkdp/bat/blob/master/tests/integration_tests.rs? I.e. invoke the binary and assert on stdout.

That way we can be sure that this contribution and use case will never stop working.

@Synthetica9 Synthetica9 changed the title Correctly render tab stops Correctly render tab stops in --show-all Jan 23, 2022
@Synthetica9
Copy link
Contributor Author

Okay, added two integration tests. Not sure about the names, feel free to suggest better ones.

@sharkdp
Copy link
Owner

sharkdp commented Feb 9, 2022

Very nice! I like it.

Could you please add an entry to the CHANGELOG? See instructions here: https://github.com/sharkdp/bat/blob/master/CONTRIBUTING.md#add-an-entry-to-the-changelog

tests/integration_tests.rs Outdated Show resolved Hide resolved
@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 Feb 24, 2022
@sharkdp sharkdp 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 Mar 6, 2022
@sharkdp sharkdp merged commit 282b702 into sharkdp:master Mar 6, 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