imsnif / diskonaut

Terminal disk space navigator 🔭
MIT License
2.48k stars 66 forks source link

Make enter select largest folder if nothing is selected #45

Closed redzic closed 4 years ago

redzic commented 4 years ago

Fixes #37 Although, the way it is now, it will only select (but not cd into) the largest folder if nothing is selected. From the wording "... the app goes into the largest folder" in the comment of #37 I wasn't really sure if it should cd into the largest folder or just select it. However, if should also cd into it, it should be a very trivial change in the handle_enter function.

redzic commented 4 years ago

It looks like the tests are failing because this PR calls slightly different operations to draw to the terminal For example:

---- tests::cases::ui::noop_when_pressing_esc_at_base_folder stdout ----
thread 'tests::cases::ui::noop_when_pressing_esc_at_base_folder' panicked at 'assertion failed: `(left == right)`
  left: `[Clear, HideCursor, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Clear, ShowCursor]`,
 right: `[Clear, HideCursor, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Draw, Flush, Clear, ShowCursor]`', src/tests/cases/ui.rs:1270:5

However, it seems like the feature is working as intended when I run it locally, so maybe the tests should be changed?

redzic commented 4 years ago

Yeah tbh, I think it's probably better if it also enters the directory because I tried using the feature and if you keep doing it multiple times it's a little inconvenient because you have to press enter twice each time. And I think I understand why the tests are failing now, thanks!

I'll make the suggested changes, thank you!

imsnif commented 4 years ago

Hey, only one more thing I forgot! Could you add a test for this? I think you can copy/paste one of the "move_X_and_enter" tests and remove the movement. What say you?

redzic commented 4 years ago

Sure, I'll try writing a test for it! Sounds not too bad if I can just copy and slightly modify already existing tests lol

imsnif commented 4 years ago

Great! I'm copy-pasting something I wrote in another thread about our snapshot tests. I hope to have this in a CONTRIBUTING.md file ASAP, but for now so you won't be stuck:

About snapshot testing with insta: we use insta (https://github.com/mitsuhiko/insta) for snapshot testing. It gives us the assert_snapshot macro. The first time it is run, it has no snapshot to compare to, so it creates one in the snapshots folder. If you install cargo-insta on your computer, you will be able to review them by doing cargo insta review after the snapshot is created. You'll see the new snapshot (in this case, the "are you sure?" modal) and be able to approve it or reject it. Once you approve it, the test should pass when you run it again.

redzic commented 4 years ago

Oops, kind of have a lot more commits than necessary because I was a little confused with the bottom asssert!s. Hopefully the test should look something like this? Thank you for the help btw!

imsnif commented 4 years ago

This is a really cool feature, I'm having a lot of fun with it in long directory chains :)

The test looks great and everything else does as well.

imsnif commented 4 years ago

Btw, no worries about the commits. I'm squashing them anyway.