-
Notifications
You must be signed in to change notification settings - Fork 12
fix: patch-hub crashes when loading patches after b4 fails #142
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
base: unstable
Are you sure you want to change the base?
fix: patch-hub crashes when loading patches after b4 fails #142
Conversation
@davidbtadokoro and @lorenzoberts we'd really appreciate it if you could check out this bug our partial "fix" introduces. It seems to have something to do with the rendering or with the thread management. Try to reproduce the original bug from #69 with the patch Do you guys have any info or what exactly we should look after in order to fix this? |
Fixed. |
167d77b
to
deba389
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few minor fixes before finishing the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good overall. But I would like to ask for a few improvements.
I'm open to help and discuss this. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi guys! I'm just dropping in to add a few comments to compliment @auyer's review (thank you so much, BTW, Rafa). Nevertheless, I can't thank you guys enough for tackling this annoying and ghostly bug! Also, feel free to ping me in case of any doubt!
Number of Commits
Because a PR is still unmerged code, we have the opportunity to refine it and make it the most atomic (along with all the other good git practices stuff).
In this sense, I feel like the first commit goes in one direction (tries to fix the bug using Result
), while the second goes in another direction (the "correct" one that works). So, you should make this PR a single commit by dropping the first and leaving just the second (it may need some adjustments).
If you don't know exactly how to do this, I recommend taking a look at this excellent section of the Git book about how you can manipulate a branch and "rewrite" history. Beyond this PR and even patch-hub, having this know-how is invaluable!
After rewriting the history locally, you need to force-push to the same remote branch, and GitHub will automatically update the PR.
Formatting and Linting
We have a formatter and a linter implemented in the project. To fix the problems pointed out by @auyer, you can use
cargo fmt --all
and
cargo lint --fix
for formatting and linting, respectively (I recommend running in this order). With the --fix
option, the linter will try to detect and also fix rule violations, but it can "fail" to automatically solve one or another problem.
Optionally, you can use the pre-commit
hook we have implemented to run both formatter and linter before every and each commit. For now, just locally and explicitly running both will suffice, IMO.
Seeking to fix kworkflow#69, this commit introduces a check to the function download_patchset and a new enum. If the function fails to create a file with the newly retrieved patch, this result is passed on and eventually treated as an alert popup. Fixes: kworkflow#69 Co-developed-by: Bruno Stephan <bruno.stephan@usp.br> Signed-off-by: Bruno Stephan <bruno.stephan@usp.br> Co-developed-by: Andre de Lima <aschwarz@usp.br> Signed-off-by: Andre de Lima <aschwarz@usp.br> Signed-off-by: Arthur Pilone <art.pilone@gmail.com>
c1aff34
to
e42de1b
Compare
Seeking to fix #69, this first commit introduces a check to the function init_details_actions. We hoped to stop the crash described in #69 by throwing an Err and catching it in the handle_latest_patchsets fucntion before showing a popup to the user. For some still unkown reason, the TUI freezes and 'melts' after the popup is shown.
Co-developed-by: Bruno Stephan bruno.stephan@usp.br
Co-developed-by: Andre de Lima aschwarz@usp.br