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

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Oct 5, 2016

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

@jlstevens

This comment has been minimized.

Member

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

This comment has been minimized.

Member

jlstevens commented Oct 6, 2016

Great, tests are passing! Merging.

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

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 72.845%
Details
s3-reference-data-cache Test data is cached.
Details
@jlstevens

This comment has been minimized.

Member

jlstevens commented Oct 7, 2016

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 Oct 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment