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

feat: add quit confirmation #89

Merged
merged 3 commits into from
Mar 23, 2024
Merged

Conversation

jondysinger
Copy link
Contributor

Implements a prompt if the user initiates a quit from the base screen.

? Really quit? (y or n) ›

Copy link
Owner

@altsem altsem 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 working on this! :)

I think it might be nice to add some simple tests for quitting (tests/mod.rs), I think something like this could work:

#[test]
fn quit_prompt() {
    let mut ctx = TestContext::setup_init(80, 20);

    let mut state = ctx.init_state();
    state.update(&mut ctx.term, &[key('q')]).unwrap();
    insta::assert_snapshot!(ctx.redact_buffer());
}

#[test]
fn quit() {
    let mut ctx = TestContext::setup_init(80, 20);
    
    // TODO init_state should probably accept `Config` as an arg?
    let mut state = ctx.init_state();
    state.update(&mut ctx.term, &[key('q'), key('y')]).unwrap();
    // Might as well just make the `quit` field `pub`
    assert!(state.quit);
}

src/state.rs Outdated Show resolved Hide resolved
src/state.rs Outdated Show resolved Hide resolved
@jondysinger
Copy link
Contributor Author

I added commits to close the prompt if the user cancels it and for the unit tests. I'm still planning on looking into having the functionality be dependent on configuration.

@jondysinger
Copy link
Contributor Author

OK I added a config entry and defaulted the behavior to not confirm the quit. I then squashed my commits. It's ready for re-review.

Copy link
Owner

@altsem altsem left a comment

Choose a reason for hiding this comment

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

Looks good and seems to work! :)
I just had a minor nitpick about the config file comment.

src/default_config.toml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 54.28571% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 85.52%. Comparing base (f526a46) to head (8405439).

Files Patch % Lines
src/state.rs 33.33% 16 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   85.84%   85.52%   -0.33%     
==========================================
  Files          37       37              
  Lines        3264     3295      +31     
==========================================
+ Hits         2802     2818      +16     
- Misses        462      477      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@altsem altsem merged commit 363703b into altsem:master Mar 23, 2024
3 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