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

Improved error reporting on comms #906

Merged
merged 7 commits into from
Oct 6, 2016
Merged

Improved error reporting on comms #906

merged 7 commits into from
Oct 6, 2016

Conversation

philippjfr
Copy link
Member

Improves the traceback reported in the console on error by including filename, error type, line number and function.

@jlstevens
Copy link
Contributor

jlstevens commented Oct 6, 2016

I'm very happy with this, especially as everything stays simple on the Javascript side.

My only comments:

  • Capture needs a docstring to explain why it is needed - to capture stdout (e.g print output) and make it available on the web console for debugging etc.
  • It probably wouldn't be too hard to capture stderr at the same time and I would consider doing this for consistency.
  • Lastly, I might consider renaming Capture to something like StandardOutput. Then the context manager can be used with:
with StandardOutput() as stdout:
     ...
if stdout: ... 

You'll need two context managers if you want to also handle stderr - at least if you want to continue using each one inheriting from list:

with StandardOutput() as stdout, StandardError() as stderr:
     ...
if stdout: ...
if stderr: ... 

The redundancy between the StandardOutput and StandardError context managers is a little annoying but maybe you could parameterize it to have StandardIO('stdout)' and StandardIO('stderr')...

@jlstevens
Copy link
Contributor

Great, tests are passing! Merging.

@jlstevens jlstevens merged commit 02f6f32 into master Oct 6, 2016
@jlstevens
Copy link
Contributor

I know this PR is merged now but I want to just state one thing that is still niggling at me. How will a regular user even know the check the Javascript console for messages? Both of us know that the output is going there now but it is invisible to everyone else until they look at the docs to find out.

Not sure how best to handle this. Javascript alerts would be very obvious, but also very annoying...though maybe for stdout, using alerts might make sense even if exceptions don't?

@philippjfr philippjfr deleted the streams_traceback branch October 14, 2016 01:55
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants