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

Error handling #48

Merged
merged 27 commits into from
Aug 14, 2023
Merged

Error handling #48

merged 27 commits into from
Aug 14, 2023

Conversation

bagel897
Copy link

The problem

When gonb runs into errors, it generates HTML error reports. These are great for a jupyter notebook, but don't render in nbmake

The solution

Add an option to print out raw error messages instead of html reports, set by a command line flag. This works inside nbmake.
image
image

@bagel897 bagel897 marked this pull request as ready for review August 11, 2023 19:32
@janpfeifer
Copy link
Owner

Thanks @bagel897 , this looks really great! Having it work well nbmake facilitates testing -- I have to start using, it also for GoNB (lots of things I didn't test because were not easy to).

A few suggestions (all very small "nit-picks"), I hope you don't mind.

  • In goexec/errorcontext.go, the small functions max, min and inBetween are all used in the new errorLine.go, so they may just be moved there.
  • errorcontext.go became really small. What do you think about merging it with error.go ? Either way is good, just something to think about.
  • Because klog injects many other flags, when reading the output of a gonb --help (or go run . --help), it's hard for someone to understand that --raw_error is about the GoNB cell execution error. Maybe comment that on the comment of the flag ? Something like (suggestion) "When GoNB executes cells, force raw text errors instead of HTML errors, which facilitates command line testing of notebooks" ?

Let me know if you would like to change them before merging (or if you disagree), or I'll just merge your PR as is do the small changes later (likely on Monday morning)

cheers, and thanks!!

@bagel897
Copy link
Author

  1. Sounds good!
  2. errorcontext.go still has the HTML template. But I understand combining the two files, it'd make it simpler. Alternatively, I can move the HTML report part back into errorcontext.go
  3. Ill do that as well

I'll do the changes monday morning, is that alright?

@janpfeifer
Copy link
Owner

janpfeifer commented Aug 12, 2023 via email

@bagel897
Copy link
Author

bagel897 commented Aug 14, 2023

Thanks for the feedback!

  1. Done
  2. I renamed it to htmlerror and moved all the HTML code back into the file. This leaves it over 100 lines, which should be good. Alternatively, we could move all the error files into their own error module, would that work better?
  3. I updated the help flag

Lmk if you have any other suggestions

Copy link
Owner

@janpfeifer janpfeifer 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 the changes!

@janpfeifer janpfeifer merged commit 1458bfe into janpfeifer:main Aug 14, 2023
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