Skip to content

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Mar 10, 2020

For #10280

@rchiodo rchiodo self-assigned this Mar 10, 2020
@codecov-io
Copy link

codecov-io commented Mar 10, 2020

Codecov Report

Merging #10511 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10511      +/-   ##
==========================================
- Coverage   60.86%   60.83%   -0.03%     
==========================================
  Files         579      579              
  Lines       31294    31296       +2     
  Branches     4451     4452       +1     
==========================================
- Hits        19046    19040       -6     
- Misses      11285    11290       +5     
- Partials      963      966       +3
Impacted Files Coverage Δ
src/datascience-ui/react-common/arePathsSame.ts 75% <0%> (-12.5%) ⬇️
src/client/common/utils/platform.ts 64.7% <0%> (-11.77%) ⬇️
src/client/linters/pydocstyle.ts 86.66% <0%> (-2.23%) ⬇️
src/client/datascience/debugLocationTracker.ts 76.56% <0%> (-1.57%) ⬇️
...ient/datascience/jupyter/kernels/kernelSelector.ts 76.37% <0%> (-1.23%) ⬇️
src/client/common/process/proc.ts 14.49% <0%> (-0.73%) ⬇️

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 cd90761...7445677. Read the comment docs.

else:
del _VSCode_supportsDataExplorer
_VSCODE_evalResult = eval(_VSCODE_targetVariable["name"])
_VSCODE_evalResult = _VSCODE_builtins.eval(_VSCODE_targetVariable["name"])
Copy link
Member

Choose a reason for hiding this comment

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

We also use builtins in VariableList and VariableValue. I'd think it would be safer to do the same thing in those files as well.

Copy link
Author

Choose a reason for hiding this comment

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

Those files aren't used anymore

Copy link
Author

Choose a reason for hiding this comment

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

I could just delete them I guess.

Copy link
Author

Choose a reason for hiding this comment

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

Actually they're still used to get variables in our ipython tests. But they don't need the update. They're just used to test the other two files. Maybe I should move them instead to the test directory

Copy link
Member

Choose a reason for hiding this comment

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

That's true, I forgot they were not used anymore. Just saw them when searching for other builtins.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
No Duplication information No Duplication information

@rchiodo rchiodo merged commit aec070b into master Mar 10, 2020
@rchiodo rchiodo deleted the rchiodo/data_float branch March 10, 2020 22:46
DonJayamanne added a commit that referenced this pull request Mar 13, 2020
* master:
  Fix flakey file system tests (#10541)
  Tweaks for gather (#10532)
  Fix #10437: Update launch.json handling to support "listen" and "connect" (#10517)
  Add conda support to nightly flake test (#10523)
  Rename datascience to datascience_modules (#10525)
  Clean up the extension activation code. (#10455)
  Allow escape and ctrl+U to clear the interactive window (#10513)
  Fix builtins so they don't interfere with our execution (#10511)
  Jupyter autocompletion will only show up on empty lines, (#10420)
  notify on missing kernel and interpreter with kernel (#10508)
@lock lock bot locked as resolved and limited conversation to collaborators Mar 17, 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.

5 participants