Skip to content

Conversation

mkatychev
Copy link
Contributor

@mkatychev mkatychev commented Oct 9, 2022

Fixes #171

  • Handles squeeze with --no-characters

  • Handles squeeze with --no-position

  • Added error handling for write! macros

  • Panels field is now usize to simplify comparing against lengths

  • If position is hidden, a trailing double asterisk is shown in the last line if it is part of a squeeze:

$ cargo run -- tests/examples/hello_world_elf64 --skip=1024 -n 4096 --plain --panels 2

  00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00
  **
  ba 0e 00 00 00 b9 00 20   40 00 bb 01 00 00 00 b8
  04 00 00 00 cd 80 b8 01   00 00 00 cd 80 00 00 00
  00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00
  **
                                                 **

@mkatychev
Copy link
Contributor Author

mkatychev commented Oct 9, 2022

@sharkdp thoughts on adding trailing double asterisks for a last line squeeze when position is turned off?

Would turn this:

$ hexyl tests/examples/hello_world_elf64 -s 1024 -n 4096 --panels 2 --plain

  00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00
  **
  ba 0e 00 00 00 b9 00 20   40 00 bb 01 00 00 00 b8
  04 00 00 00 cd 80 b8 01   00 00 00 cd 80 00 00 00
  00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00
  **
  

Into this:

$ hexyl tests/examples/hello_world_elf64 -s 1024 -n 4096 --panels 2 --plain

  00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00
  **
  ba 0e 00 00 00 b9 00 20   40 00 bb 01 00 00 00 b8
  04 00 00 00 cd 80 b8 01   00 00 00 cd 80 00 00 00
  00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00
  **
                                                 **

EDIT: this is now implemented

@mkatychev
Copy link
Contributor Author

mkatychev commented Oct 10, 2022

There ended up being a lot of edge cases toggling chars, position, trailing byte padding, and number of panels.
I ended up adding a trait to the Assert object to help with diffing whitespace (and pretty_assertions as a dev dependency)

// https://github.com/assert-rs/assert_cmd/issues/121#issuecomment-849937376
//
impl<S> PrettyAssert<S> for assert_cmd::assert::Assert
where
S: AsRef<str>,
{
fn pretty_stdout(self, other: S) {
println!("{}", other.as_ref().len());
let self_str = String::from_utf8(self.get_output().stdout.clone()).unwrap();
println!("{}", self_str.len());
pretty_assertions::assert_eq!(self_str, other.as_ref());
}
}

so that this:

  ---- display_settings::no_chars_squeeze stdout ----
thread 'display_settings::no_chars_squeeze' panicked at 'Unexpected stdout, failed diff original var
├── original: ┌────────┬─────────────────────────┬─────────────────────────┐
│   │00000400│ 00 00 00 00 00 00 00 00 ┊ 00 00 00 00 00 00 00 00 │
│   │*       │                         ┊                         │
│   │00001000│ ba 0e 00 00 00 b9 00 20 ┊ 40 00 bb 01 00 00 00 b8 │
│   │00001010│ 04 00 00 00 cd 80 b8 01 ┊ 00 00 00 cd 80 00 00 00 │
│   │00001020│ 00 00 00 00 00 00 00 00 ┊ 00 00 00 00 00 00 00 00 │
│   │*       │                         ┊                         │
│   │00001400│                         ┊                         │
│   └────────┴─────────────────────────┴─────────────────────────┘
├── diff:
│   ---         orig
│   +++         var
│   @@ -8 +8 @@
│   -│00001400│                         ┊                         │
│   +│00001400│                         ┊                         │
└── var as str: ┌────────┬─────────────────────────┬─────────────────────────┐
    │00000400│ 00 00 00 00 00 00 00 00 ┊ 00 00 00 00 00 00 00 00 │
    │*       │                         ┊                         │
    │00001000│ ba 0e 00 00 00 b9 00 20 ┊ 40 00 bb 01 00 00 00 b8 │
    │00001010│ 04 00 00 00 cd 80 b8 01 ┊ 00 00 00 cd 80 00 00 00 │
    │00001020│ 00 00 00 00 00 00 00 00 ┊ 00 00 00 00 00 00 00 00 │
    │*       │                         ┊                         │
    │00001400│                         ┊                         │
    └────────┴─────────────────────────┴─────────────────────────┘

                                                 **

can turn into this:

image

More context here:
assert-rs/assert_cmd#121 (comment)

@mkatychev mkatychev marked this pull request as ready for review October 10, 2022 02:13
@mkatychev
Copy link
Contributor Author

@sharkdp Are we anticipating #173 being merged first?

@sharkdp
Copy link
Owner

sharkdp commented Oct 14, 2022

@sharkdp Are we anticipating #173 being merged first?

Yes, I think that would be the best strategy overall. Sorry for that :-/.

Thank you for your PR, I will take a look soon.

@sharkdp
Copy link
Owner

sharkdp commented Nov 13, 2022

#173 has now been merged, but please note that there are a few more open PRs that touch similar parts of the code as well (e.g. #170). Sorry :-/

@mkatychev
Copy link
Contributor Author

mkatychev commented Nov 18, 2022

Understandable, it will take me a while to get to the conflicts at the moment unfortunately.
@sharkdp I'll try to revisit this next weekend.

@sharifhsn
Copy link
Contributor

For what it's worth, most of the issues covered by this PR have been fixed by the larger architectural changes. There are no longer any artifacts when lines are printed, and squeezing is properly handled in all cases. I've also standardized error handling to use ? syntax bubbling up io::Result<()>. There are only a few differences in this PR:

  1. A single asterisk is printed on the squeeze line when the position panel is hidden. Changing this to a double asterisk is trivial.
  2. The feature-full testing framework added into the integration tests. This will not conflict with any other recently merged or soon to be merged PR.

If there's anything I missed, let me know.

@sharkdp
Copy link
Owner

sharkdp commented Nov 19, 2022

A single asterisk is printed on the squeeze line when the position panel is hidden. Changing this to a double asterisk is trivial.

Okay. I'm fine with either version. So let's keep what we have right now, unless this double asterisk has some precedent in other apps. Or if there is any other advantage.

The feature-full testing framework added into the integration tests. This will not conflict with any other recently merged or soon to be merged PR.

@mkatychev Do you think you could extract those changes into a new PR?

@sharifhsn Or are these things already covered in your new unit tests for squeezing?

@sharifhsn
Copy link
Contributor

sharifhsn commented Nov 19, 2022

There are some cases not covered by my tests, like squeeze with incomplete last line. And in general, more testing is a good thing, especially since all my tests were unit, not integration. The only changes would have to be the asterisks when printing without position panel.

@mkatychev
Copy link
Contributor Author

@sharkdp I'll start with the tests in a new branch and see what breaks then.

@mkatychev mkatychev closed this Nov 26, 2022
@mkatychev mkatychev force-pushed the fix/no_char_no_pos_squeeze branch from fed8d5c to fe65792 Compare November 26, 2022 17:53
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.

--no-characters causes output artifacts when squeezing lines
3 participants