Skip to content

Conversation

@MikhailArkhipov
Copy link

Fixes #1013
Fixes #1776

This pull request:

  • Has a title summarizes what is changing
  • Has unit tests & code coverage is not adversely affected (within reason)
  • Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
  • Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)

PTVS PR microsoft/PTVS#4354

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

I'll test this tomorrow with the integration with the code lenses for Unit tests.
Will approve once that functionality works as expected.

let pythonPath = (await envProvider.getEnvironmentVariables()).PYTHONPATH;
this.interpreterHash = interpreterData ? interpreterData.hash : '';

searchPaths = searchPaths.replace(/\\\\/g, '\\');

Choose a reason for hiding this comment

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

Why are we doing this? Please could you document the use case.
Is this for Windows Only? You might want to consider using path.normalize(searchPaths)
That function removes unnecessary (duplicate) path separators.

Copy link
Author

Choose a reason for hiding this comment

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

OK, will use normalize. I think it is somewhere is node when converting settings in JSON. Generally it doesn't matter for file system operations as both .NET and node ignore duplicate backslashes. However, it matters in the language server that is tracking documents by URI and different number of backslashes produces different URI strings.

Copy link
Author

Choose a reason for hiding this comment

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

There is, however, difficultly with search paths since it is a single string which would have to be split and re-joined.

@codecov
Copy link

codecov bot commented Jun 12, 2018

Codecov Report

Merging #1934 into master will increase coverage by 0.4%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1934     +/-   ##
=========================================
+ Coverage   74.35%   74.75%   +0.4%     
=========================================
  Files         283      285      +2     
  Lines       13287    13456    +169     
  Branches     2387     2426     +39     
=========================================
+ Hits         9879    10059    +180     
+ Misses       3273     3266      -7     
+ Partials      135      131      -4
Impacted Files Coverage Δ
src/client/extension.ts 95.17% <ø> (-0.07%) ⬇️
src/client/activation/analysis.ts 18.39% <0%> (-0.3%) ⬇️
src/client/activation/classic.ts 91.83% <100%> (+0.53%) ⬆️
src/client/common/installer/types.ts 100% <0%> (ø) ⬆️
src/client/common/installer/serviceRegistry.ts 100% <0%> (ø) ⬆️
src/client/common/types.ts 100% <0%> (ø) ⬆️
src/client/common/installer/productPath.ts 100% <0%> (ø)
src/client/common/installer/productService.ts 100% <0%> (ø)
src/client/debugger/mainV2.ts 78.71% <0%> (+0.8%) ⬆️
src/client/linters/lintingEngine.ts 91.15% <0%> (+0.88%) ⬆️
... and 3 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 3a614bc...279a8e7. Read the comment docs.

@MikhailArkhipov MikhailArkhipov merged commit 9a520ca into microsoft:master Jun 13, 2018
@MikhailArkhipov MikhailArkhipov deleted the gotodef branch September 13, 2018 01:29
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
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.

Go to definition is not available in new Analysis engine Document symbol via Language Server

2 participants