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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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 10 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
Member

@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.

Comment on lines +353 to +355
/// // or with a function
/// terminal.try_draw(render)?;
///
Copy link
Member

Choose a reason for hiding this comment

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

Either empty lines between both or no empty lines. It took me a moment to realise where render is defined as the comment looked like its somewhere completely else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you looking at the rendered docs?

image

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
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.

None yet

3 participants