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

@philippjfr philippjfr commented Oct 5, 2016

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

@philippjfr philippjfr force-pushed the streams_traceback branch from 17d69ab to 63c1460 Oct 5, 2016
@jlstevens
Copy link
Contributor

@jlstevens 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')...

@philippjfr philippjfr force-pushed the streams_traceback branch from b7ac1b1 to e2c55ec Oct 6, 2016
@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Oct 6, 2016

Great, tests are passing! Merging.

@jlstevens jlstevens merged commit 02f6f32 into master Oct 6, 2016
4 checks passed
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
@philippjfr
s3-reference-data-cache Test data is cached.
Details
@jlstevens
Copy link
Contributor

@jlstevens 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants