Skip to content

Error line numbers coming out with invalid values #11234

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

Merged
merged 4 commits into from
Apr 20, 2020

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Apr 17, 2020

For #10708

Number of problems here.

Old code assumed that if the file in the error message matched the cell, it was the cell. This doesn't work if the exception is thrown in a different cell that's been referenced (as it will have the same file). The example for this is in the bug.

Old code also didn't take into account extra spacing at the top of a cell.

New code uses the cell hash provider to look for the closest match to the source code in the stack trace (if the file matches a file that we ran cells from).

Note that this may find invalid cells if they have duplicate code in them. Don't see a way around this without getting rid of the __file__ support. An exception doesn't log any other information other than the code in the traceback.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 2 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov-io
Copy link

codecov-io commented Apr 17, 2020

Codecov Report

Merging #11234 into master will decrease coverage by 0.29%.
The diff coverage is 82.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11234      +/-   ##
==========================================
- Coverage   61.37%   61.08%   -0.30%     
==========================================
  Files         604      601       -3     
  Lines       33528    33108     -420     
  Branches     4746     4678      -68     
==========================================
- Hits        20577    20223     -354     
+ Misses      11894    11865      -29     
+ Partials     1057     1020      -37     
Impacted Files Coverage Δ
src/client/datascience/jupyter/jupyterUtils.ts 57.14% <ø> (-17.28%) ⬇️
src/client/datascience/types.ts 100.00% <ø> (ø)
src/client/datascience/jupyter/jupyterNotebook.ts 4.21% <50.00%> (-0.01%) ⬇️
...datascience/editor-integration/cellhashprovider.ts 77.65% <84.09%> (+4.74%) ⬆️
.../datascience/jupyter/interpreter/jupyterCommand.ts 9.48% <0.00%> (-45.69%) ⬇️
src/client/common/errors/errorUtils.ts 50.00% <0.00%> (-16.67%) ⬇️
src/datascience-ui/react-common/arePathsSame.ts 75.00% <0.00%> (-12.50%) ⬇️
src/client/common/utils/platform.ts 64.70% <0.00%> (-11.77%) ⬇️
...t/datascience/jupyter/kernels/jupyterKernelSpec.ts 92.00% <0.00%> (-8.00%) ⬇️
src/client/common/cancellation.ts 69.38% <0.00%> (-4.09%) ⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bed560...dea4fce. Read the comment docs.

@IanMatthewHuff
Copy link
Member

Sorry got wrapped up in the bug bash. I can look early monday.

let sourceLines = '';
const regex = /(;32m[ ->]*?)(\d+)(.*)/g;
for (let l = regex.exec(traceFrame); l && l.length > 3; l = regex.exec(traceFrame)) {
const newLine = stripAnsi(l[3]).substr(1); // Seem to have a space on the front
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments like this make me worry this is a bit fragile of a process. But I don't see a way around it, so it might just be what we have to do.

@rchiodo rchiodo merged commit 332d3fa into master Apr 20, 2020
@rchiodo rchiodo deleted the rchiodo/error_line_numbers branch April 20, 2020 16:06
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
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.

4 participants