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(terminal): add Terminal::try_draw() method #1209

Merged
merged 3 commits into from
Jul 9, 2024
Merged

feat(terminal): add Terminal::try_draw() method #1209

merged 3 commits into from
Jul 9, 2024

Conversation

joshka
Copy link
Member

@joshka joshka commented Jun 28, 2024

This makes it easier to write fallible rendering methods that can use the ? operator

terminal.try_draw(|frame| {
    some_method_that_can_fail()?;
    another_faillible_method()?;
    Ok(())
})?;

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.4%. Comparing base (3f2f2cd) to head (5f4e508).
Report is 14 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff           @@
##            main   #1209    +/-   ##
======================================
  Coverage   94.4%   94.4%            
======================================
  Files         62      62            
  Lines      14941   15056   +115     
======================================
+ Hits       14110   14225   +115     
  Misses       831     831            

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

This makes it easier to write fallible rendering methods which can use the `?` operator
Copy link
Collaborator

@kdheepak kdheepak left a comment

Choose a reason for hiding this comment

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

Cool idea! LGTM! Curious what other people think.

@joshka
Copy link
Member Author

joshka commented Jun 28, 2024

Cool idea! LGTM! Curious what other people think.

@EdJoPaTo ?

Copy link
Contributor

@EdJoPaTo EdJoPaTo left a comment

Choose a reason for hiding this comment

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

This simplifies the rendering but I'm not sure how useful it actually might be. Ideally the same Render Widget or whatever its named trait should exist here too I think? That would reduce the differences. Especially as a context is of interest anyway and Frame would reduce the need for the context too.
And the current trait doesn't allow for a Result currently either? Not sure what the ideal way would be.

src/terminal/terminal.rs Show resolved Hide resolved
src/terminal/terminal.rs Outdated Show resolved Hide resolved
@joshka
Copy link
Member Author

joshka commented Jun 28, 2024

This simplifies the rendering but I'm not sure how useful it actually might be. Ideally the same Render Widget or whatever its named trait should exist here too I think? That would reduce the differences. Especially as a context is of interest anyway and Frame would reduce the need for the context too.
And the current trait doesn't allow for a Result currently either? Not sure what the ideal way would be.

I regularly find myself using methods which return results and having to throw away the possible errors inside of the draw method by either crashing or ignoring them. This would make it possible to add more context to the errors (using anyhow / eyre), and have the app more gracefully crash.

This is a first point in making some of these future things more useful. We don't need to solve the entire problem, but this is a piece that is fine and necessary on its own.

src/terminal/terminal.rs Outdated Show resolved Hide resolved
@joshka joshka merged commit 3bb374d into main Jul 9, 2024
33 checks passed
@joshka joshka deleted the jm/try-draw branch July 9, 2024 07:46
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