Print non-printable characters using caret notation #2443

einfachIrgendwer0815 · 2023-01-07T13:08:47Z

Implements feature request #2429.

When using --show-all, non-printable characters are printed using the Unicode Control Pictures. This pr adds the flag --caret-notation, which can only be used together with --show-all. When --caret-notation is set, non-printable characters are printed using caret notation instead.

For example, without --caret-notation:

This·is·an·example·text.␊␍

With --caret-notation:

This·is·an·example·text.^J^M

Instead of --caret-notation, the alias --caret can be used too.

When this flag is set, non-printable characters are printed using caret notation.

Enselic

Thanks, looks sensible on first glance! Before a detailed review, please also add regression tests to integration_tests.rs

Enselic · 2023-01-07T13:16:34Z

src/preprocessor.rs

+                            '\x1B' => output.push_str("^["),
+
+                            // everything else
+                            c => output.push_str(&c.escape_unicode().collect::<String>())


To reduce the diff and levels of indentation I would prefer if you added if use_caret to each existing character arm instead is of doing the "refactoring" in the current PR

I'm sorry, I don't quite understand your comment. So should I refactor it or not?

Sorry for being vague. I mean that I would prefer e.g.

'\x1B' => output.push('␛'),

to be changed to

'\x1B' => output.push_str(if use_caret { "^[" } else { "␛" }),

(+ formatted however cargo fmt prefers it)

Enselic · 2023-01-15T16:22:57Z

Thank you for the update. I will review as soon as I get some time over.

Enselic

This looks good! To be cautious, I would like us to not have the --caret alias, at least not in the initial version. Otherwise this is Approved from me.

Enselic · 2023-01-21T20:58:02Z

src/bin/bat/clap_app.rs

+        .arg(
+            Arg::new("caret-notation")
+                .long("caret-notation")
+                .alias("caret")


I would prefer if we didn't make --caret a short form of --caret-notation. "caret notation" indeed seems to be the official term for this, but I don't think --caret is clear enough on its own. I wouldn't rule out us adding an alias in the future, but since it is much easier to add stuff than to remove stuff, I would prefer if we started out without it.

Enselic

👍

sharkdp · 2023-01-29T13:34:20Z

This looks good - thank you! Just one question regarding the CLI. To avoid introducing (1) a new option and (2) a new option that can only be used together with another option, we could also think about modifying the --show-all option. We could change it such that it takes a value (the "non printable character style"). For now, this could have two options: unicode and caret/caret-notation. The default could be unicode to maintain backwards-compatibility.

Downside: this might break cat compatibility in some edge cases?!

Another downside is that it looks a bit strange: --show-all=caret seems to imply something else (show all carets). If we like the idea above, another possibility would be to introduce a new option (like suggested here), but a bit more future-proof. Something like --show-nonprintables-as=unicode/caret. In this case, --show-all would be an alias for --show-nonprintables=unicode.

What do others think of this option?

Enselic · 2023-01-29T14:14:55Z

I think we should leave --show-all as is for compatibility with cat.

But I like the idea of making the name of the new option more future-proof. Both OpenBSD cat and GNU cat use the terminology nonprinting, so I propose that we rename --caret-notation to --nonprinting-notation=caret, with --nonprinting-notation=unicode being default.

Sorry for changing my mind, but I think it would be good to make the new option future proof!

Enselic

Since this will affect the public API of the bat library, it is worth putting some extra effort into designing the public API.

My preferred way to see how the public API is impacted is to run cargo public-api diff latest after installing with cargo install --locked cargo-public-api.

Here is the output I get when diffing the public API of your branch against the public API of the latest bat release on crates.io (in the terminal there is syntax highlighting, but that gets lost on GitHub):

$ cargo public-api diff latest
Resolved `diff latest` to `diff 0.22.1`
 Documenting bat v0.22.1
    Finished dev [unoptimized + debuginfo] target(s) in 0.56s
 Documenting bat v0.22.1 (/home/martin/src/bat)
    Finished dev [unoptimized + debuginfo] target(s) in 0.63s
Removed items from the public API
=================================
(none)

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

Added items to the public API
=============================
+pub bat::config::Config::nonprinting_notation: alloc::string::String
+pub fn bat::style::StyleComponents::clear(&mut self)
+pub fn bat::style::StyleComponents::insert(&mut self, component: bat::style::StyleComponent)

As we can see, bat::config::Config::nonprinting_notation: alloc::string::String has been added to the public API.

I really think we should use an enum instead of a free form String. See e.g. https://docs.rs/bat/latest/bat/enum.WrappingMode.html.

So can you introduce an enum please? Thanks!

Enselic · 2023-02-11T15:36:50Z

Thank you for the reminder. I am unusually busy both privately and professionally, so I do not have time to look againt at this PR right now.

I have not forgotten about it though. I would be surprised if it takes longer than a month before I get some time over. (Or maybe some other maintainer will take a look before that.)

Enselic

I just realized we already use the term nonprintable in Config::show_nonprintable. So to be consistent with ourselves, I think we should rename NonprintingNotation to NonprintableNotation.

Sorry for changing my mind again :(

Enselic · 2023-02-26T16:08:11Z

src/nonprinting_notation.rs

+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum NonprintingNotation {
+    Caret,
+    Unicode,
+}
+
+impl Default for NonprintingNotation {
+    fn default() -> Self {
+        NonprintingNotation::Unicode
+    }
+}


Can you add docs and automatically derive Default please? Proposal:

/// How to print non-printable characters with /// [crate::config::Config::show_nonprintable] #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] pub enum NonprintingNotation { /// Use unicode notation (␇, ␊, ␀, ..) #[default] Unicode, /// Use caret notation (^G, ^J, ^@, ..) Caret, }

einfachIrgendwer0815 · 2023-03-02T18:21:16Z

Can you add docs and automatically derive Default please? Proposal:

/// How to print non-printable characters with
/// [crate::config::Config::show_nonprintable]
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
pub enum NonprintingNotation {
    /// Use unicode notation (␇, ␊, ␀, ..)
    #[default]
    Unicode,

    /// Use caret notation (^G, ^J, ^@, ..)
    Caret,
}

As the failed check shows, this is not compatible with the minimum supported Rust version (1.60), as this feature wasn't stabilized until version 1.62

Enselic · 2023-03-03T06:10:50Z

As the failed check shows, this is not compatible with the minimum supported Rust version (1.60), as this feature wasn't stabilized until version 1.62

Oh, I see. Well, that's annoying. Let's bump MSRV then! PR: #2496

Enselic

Thanks a lot for the updates, this looks good to me now!

For the record, here is the bat public API diff:

$ cargo public-api --omit blanket-impls --omit auto-trait-impls --omit auto-derived-impls diff origin/master..einfachIrgendwer0815/feature_caret_notation
Removed items from the public API
=================================
(none)

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

Added items to the public API
=============================
+pub bat::config::Config::nonprintable_notation: bat::NonprintableNotation
+pub enum bat::NonprintableNotation
+pub bat::NonprintableNotation::Caret
+pub bat::NonprintableNotation::Unicode

Enselic · 2023-03-10T15:49:44Z

src/nonprintable_notation.rs

+/// How to print non-printable characters with
+/// [crate::config::Config::show_nonprintable]
+#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
+pub enum NonprintableNotation {


I forgot one thing! We need to make the new enum #[non_exhaustive]. Otherwise adding more notations will require a major semver version bump.

Suggested change

pub enum NonprintableNotation {

#[non_exhaustive]

pub enum NonprintableNotation {

You don't have to add it though. I can add it before I merge this. I plan on merging it early next week, unless something else comes up.

einfachIrgendwer0815 added 2 commits January 7, 2023 13:37

Add --caret-notation flag

068aaaf

When this flag is set, non-printable characters are printed using caret notation.

Update CHANGELOG

0c8816e

Enselic reviewed Jan 7, 2023

View reviewed changes

einfachIrgendwer0815 added 2 commits January 7, 2023 14:57

Add test for --caret-notation

a71af3b

Correct code formatting

5824e5d

Repository owner deleted a comment from leandro86elpiojo Jan 9, 2023

Refactor to reduce levels of indentation

13b53b1

einfachIrgendwer0815 requested a review from Enselic January 15, 2023 15:27

Enselic reviewed Jan 21, 2023

View reviewed changes

einfachIrgendwer0815 and others added 5 commits January 23, 2023 18:52

Remove alias for --caret-notation

dfc390a

Update CHANGELOG according to dfc390a

e98d3e6

Cherry-pick 5cd7766

df6b893

Comply with requirements added with pr sharkdp#2430

bbec966

Merge branch 'master' into feature_caret_notation

ae9cf9d

Enselic previously approved these changes Jan 28, 2023

View reviewed changes

einfachIrgendwer0815 added 2 commits February 5, 2023 10:36

Rename --caret-notation to --nonprinting-notation=caret

dc15283

Update comment

f5dbfb3

Enselic requested changes Feb 5, 2023

View reviewed changes

Add an enum for possible --nonprinting-notation values

eca1be7

einfachIrgendwer0815 requested a review from Enselic February 11, 2023 15:29

Enselic requested changes Feb 26, 2023

View reviewed changes

einfachIrgendwer0815 added 3 commits February 28, 2023 14:07

Rename 'nonprinting-notation' to 'nonprintable-notation'

5a1cc9e

Automatically derive Default for NonprintableNotation

80d8ed2

Add docs for NonprintableNotation

76f18a6

Enselic mentioned this pull request Mar 3, 2023

Bump MSRV to 1.62 #2496

Merged

Merge remote-tracking branch 'origin/master' into feature_caret_notation

619d9c7

Enselic approved these changes Mar 4, 2023

View reviewed changes

Enselic requested changes Mar 10, 2023

View reviewed changes

Mark NonprintableNotation #[non_exhaustive]

ddcc6cf

Enselic approved these changes Mar 14, 2023

View reviewed changes

Enselic merged commit 8f99a78 into sharkdp:master Mar 14, 2023

einfachIrgendwer0815 mentioned this pull request Oct 15, 2023

Add unicode/caret replacements for all control characters #2712

Merged

einfachIrgendwer0815 deleted the feature_caret_notation branch February 24, 2024 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print non-printable characters using caret notation #2443

Print non-printable characters using caret notation #2443

einfachIrgendwer0815 commented Jan 7, 2023

Enselic left a comment

Enselic Jan 7, 2023

einfachIrgendwer0815 Jan 7, 2023

Enselic Jan 8, 2023 •

edited

Loading

Enselic commented Jan 15, 2023

Enselic left a comment

Enselic Jan 21, 2023 •

edited

Loading

Enselic left a comment

sharkdp commented Jan 29, 2023 •

edited

Loading

Enselic commented Jan 29, 2023

Enselic left a comment

Enselic commented Feb 11, 2023

Enselic left a comment

Enselic Feb 26, 2023 •

edited

Loading

einfachIrgendwer0815 commented Mar 2, 2023

Enselic commented Mar 3, 2023

Enselic left a comment •

edited

Loading

Enselic Mar 10, 2023

	pub enum NonprintableNotation {
	#[non_exhaustive]
	pub enum NonprintableNotation {

Print non-printable characters using caret notation #2443

Print non-printable characters using caret notation #2443

Conversation

einfachIrgendwer0815 commented Jan 7, 2023

Enselic left a comment

Choose a reason for hiding this comment

Enselic Jan 7, 2023

Choose a reason for hiding this comment

einfachIrgendwer0815 Jan 7, 2023

Choose a reason for hiding this comment

Enselic Jan 8, 2023 • edited Loading

Choose a reason for hiding this comment

Enselic commented Jan 15, 2023

Enselic left a comment

Choose a reason for hiding this comment

Enselic Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

Enselic left a comment

Choose a reason for hiding this comment

sharkdp commented Jan 29, 2023 • edited Loading

Enselic commented Jan 29, 2023

Enselic left a comment

Choose a reason for hiding this comment

Enselic commented Feb 11, 2023

Enselic left a comment

Choose a reason for hiding this comment

Enselic Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

einfachIrgendwer0815 commented Mar 2, 2023

Enselic commented Mar 3, 2023

Enselic left a comment • edited Loading

Choose a reason for hiding this comment

Enselic Mar 10, 2023

Choose a reason for hiding this comment

Enselic Jan 8, 2023 •

edited

Loading

Enselic Jan 21, 2023 •

edited

Loading

sharkdp commented Jan 29, 2023 •

edited

Loading

Enselic Feb 26, 2023 •

edited

Loading

Enselic left a comment •

edited

Loading