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

Use empty history if search query fails #690

Closed
wants to merge 4 commits into from

Conversation

ysthakur
Copy link
Member

There's a bunch of places with .expect("todo: error handling"), and this PR tries to remove most of them. Here's the four different kinds:

  • There are a couple inside examples/demo.rs that I didn't bother with.
  • Some of these are for unwrapping the result of running a search query to look through the history. I figured that instead of panicking, it may be better to silently treat Errs as the history being empty.
  • One of them comes after history.save(entry), in engine.rs. This has been edited to silently ignore an Err.
  • The rest of the .expects are all inside engine.rs and are for handling the result of moving the history_cursor forward or backward. Unfortunately, all of them appear inside methods that return either () or an io::Result. These have also been edited to silently ignore Errs.

A disadvantage of silently handling Errs is that users won't know that something went wrong, but if the .expects are to be kept, they should at least be changed to have more useful messages to help users file bug reports.

There shouldn't be any user-facing changes from this.

Copy link

codecov bot commented Dec 27, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (f396223) 49.26% compared to head (0dd3f7d) 49.34%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #690      +/-   ##
==========================================
+ Coverage   49.26%   49.34%   +0.08%     
==========================================
  Files          46       46              
  Lines        7911     7897      -14     
==========================================
  Hits         3897     3897              
+ Misses       4014     4000      -14     
Files Coverage Δ
src/hinter/default.rs 0.00% <0.00%> (ø)
src/completion/history.rs 0.00% <0.00%> (ø)
src/engine.rs 5.06% <0.00%> (+0.06%) ⬆️

@amtoine
Copy link
Member

amtoine commented Dec 28, 2023

silent errors sound worse to me than panicking 🤔
would be nice to give a proper error to the users

@ysthakur
Copy link
Member Author

@amtoine Hmm, I guess so. I'm going to try fuzzing later to try to find cases that trigger panics, and if I can't find anything, I'll just change the expect messages to be more descriptive

@ysthakur ysthakur marked this pull request as draft December 28, 2023 15:35
@ysthakur
Copy link
Member Author

I did some fuzzing in this branch and couldn't find any real defects, so I guess I'll just change the error messages to be more descriptive, there's probably no need to do actual error handling.

@ysthakur
Copy link
Member Author

Actually, I have no idea what kind of message would be helpful there, so I think I'll just close this for now, lol

@ysthakur ysthakur closed this Dec 29, 2023
@ysthakur ysthakur deleted the remove-expects branch December 29, 2023 20:46
@fdncred
Copy link
Collaborator

fdncred commented Dec 29, 2023

darn, i was hoping this pr would get rid of some of the non-error-handling we have. maybe in the future sometime?

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