Added FSWatching base class and made related changes #14605

karrtikr · 2020-11-04T01:16:37Z

Created a work item to move all low level locators separately.

codecov-io · 2020-11-04T01:36:17Z

Codecov Report

Merging #14605 into main will decrease coverage by 64.89%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main   #14605       +/-   ##
===========================================
- Coverage   64.89%    0.00%   -64.90%     
===========================================
  Files         541        4      -537     
  Lines       25156       11    -25145     
  Branches     3545        0     -3545     
===========================================
- Hits        16324        0    -16324     
+ Misses       8170       11     -8159     
+ Partials      662        0      -662

Impacted Files	Coverage Δ
src/client/debugger/constants.ts	`0.00% <0.00%> (-100.00%)`	⬇️
src/client/common/process/constants.ts	`0.00% <0.00%> (-100.00%)`	⬇️
src/client/common/platform/constants.ts	`0.00% <0.00%> (-100.00%)`	⬇️
src/client/interpreter/autoSelection/constants.ts	`0.00% <0.00%> (-100.00%)`	⬇️
src/client/interpreter/display/progressDisplay.ts
src/client/terminals/activation.ts
src/client/formatters/helper.ts
src/client/testing/codeLenses/main.ts
src/client/common/experiments/groups.ts
src/client/common/process/proc.ts
... and 526 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 f752706...21b740a. Read the comment docs.

src/client/common/platform/fileSystemWatcher.ts

ericsnowcurrently

Thanks for working on this, @karrtikr. I think you're on the right track. My main recommendation is that you give FSWatchingLocator a meaningful purpose. Otherwise I don't think there is anything too major here, though I've noted a few places where I have concerns about correctness.

src/client/common/platform/fileSystemWatcher.ts

src/client/pythonEnvironments/discovery/locators/services/globalVirtualEnvronmentLocator.ts

src/client/pythonEnvironments/base/locators/lowLevel/locator.ts

src/client/pythonEnvironments/discovery/locators/services/globalVirtualEnvronmentLocator.ts

src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts

ericsnowcurrently

I've left a few relatively minor comments for which I'd appreciate a resolution before this gets merged.

Otherwise, LGTM.

src/client/common/syncDisposableRegistry.ts

src/client/pythonEnvironments/base/locator.ts

src/client/common/syncDisposableRegistry.ts

ericsnowcurrently · 2020-11-05T23:16:21Z

src/client/pythonEnvironments/base/locators/lowLevel/fsWatchingLocator.ts

+        /**
+         * Location(s) to watch for python binaries.
+         */
+        private readonly getRoots: () => Promise<string[]> | string,


Suggested change

private readonly getRoots: () => Promise<string[]> | string,

private readonly getRoots: () => Promise<string[]> | string[],

Actually it's getRoot(s) as explained in the doc. Not every locator has an array of locations they want to search (getWindowsAppsRoot), some just have a single location, so this is just convenience.

I have added the string[] type but not removed string.

If they have a single location then they return a single element array. Returning string means that the caller always has to check typeof result === 'string'. So it makes things simpler if we only return Promise<string[]> | string[].

(Also note that if you support return string then the return type should be Promise<string[] | string> | string[] | string. However, as I said, I don't think the distinction between string[] and string adds any value while it increases complexity.

Returning string means that the caller always has to check typeof result === 'string'

I don't think so.. not sure why you say that. The caller for the "string" case here just passes in

constructor() { super( getWindowsStoreAppsRoot, async () => this.kind, { executableBaseGlob: pythonExeGlob }, ); }

While in string[] case

constructor() { super( () => [getWindowsStoreAppsRoot()], async () => this.kind, { executableBaseGlob: pythonExeGlob }, ); }

So adding the string type decreases complexity for the caller. Note this is just convenience.

Ah, I see the problem. I was expecting it to return one of the following (and I believe it does):

() => Promise<string[]>

() => string[]

() => string

While you appear to be thinking it is:

() => Promise<string[]>

string[]

string

I'm pretty sure the correct form to also meet your expectations is:

string[] | string | () => Promise<string[]> | string[]

Though parentheses might be needed:

string[] | string | (() => Promise<string[]> | string[])

FWIW, I agree that the string[] | string types are convenient and should be okay.

src/client/pythonEnvironments/base/locators/lowLevel/fsWatchingLocator.ts

sonarqubecloud · 2020-11-05T23:58:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

* Add FSWatching locator base class * Correct glob pattern to not match python3.2whoa * Add documentation of python binary watcher * Fix lint errors * Update ignore list * Add disposable registry * Modify FSWatching Locator * Code reviews * Use string[] * Remove list disposable getter

* Added FSWatching base class and made related changes (#14605) * Add FSWatching locator base class * Correct glob pattern to not match python3.2whoa * Add documentation of python binary watcher * Fix lint errors * Update ignore list * Add disposable registry * Modify FSWatching Locator * Code reviews * Use string[] * Remove list disposable getter * Fix failing global virtual env watcher tests (#14633) Co-authored-by: Kartik Raj <[email protected]>

* Cherry pick from main (#14644) * Update shipped wheels version (#14615) * Update shipped wheels version * News item * Remove redundant files (#14620) * Add extension dependencies at build time (#14636) * Use Node 12.15 in Insiders and Release GitHub Actions (#14641) * Use Node 12.15 on all Insiders and Release GitHub Actions jobs (#14642) Co-authored-by: Joyce Er <[email protected]> * Cherry pic fixes into release for tests. (#14673) * Added FSWatching base class and made related changes (#14605) * Add FSWatching locator base class * Correct glob pattern to not match python3.2whoa * Add documentation of python binary watcher * Fix lint errors * Update ignore list * Add disposable registry * Modify FSWatching Locator * Code reviews * Use string[] * Remove list disposable getter * Fix failing global virtual env watcher tests (#14633) Co-authored-by: Kartik Raj <[email protected]> * Version, change log and cherrypicks for nov release (#14696) * change log updates * Update gifs * Fix for interpreter selection (#14693) * Fix for interpreter selection * Fix linting errors * Minor tweak to property removal * Cherry pick "Bind function to correct this for workspace syms" (#14743) * Fix #14674: Enable overriding "pythonPath" in the launcher Fix #12462: Update launch.json schema to add "python" and remove "pythonPath" Split the "pythonPath" debug property into "python", "debugAdapterPython", and "debugLauncherPython". Do most debug config validation on fully expanded property values via resolveDebugConfigurationWithSubstitutedVariables(). Add fixups for legacy launch.json with "pythonPath". * Point release change log and version update (#14750) * Point release change log and version update * Fix process picker (#14700) * Workaround VSCode bug for process picker * Fix how we pass in icons to VSCode * update change log with cherry pick Co-authored-by: Kartik Raj <[email protected]> Co-authored-by: Joyce Er <[email protected]> Co-authored-by: Kartik Raj <[email protected]> Co-authored-by: Jake Bailey <[email protected]> Co-authored-by: Pavel Minaev <[email protected]>

Kartik Raj added 2 commits November 3, 2020 16:12

Add FSWatching locator base class

ad675c1

Correct glob pattern to not match python3.2whoa

06e6f5b

karrtikr added the no-changelog No news entry required label Nov 4, 2020

auto-assign bot requested review from ericsnowcurrently and karthiknadig November 4, 2020 01:17

Kartik Raj added 2 commits November 3, 2020 21:48

Add documentation of python binary watcher

c167f57

Fix lint errors

375df6b

karthiknadig approved these changes Nov 4, 2020

View reviewed changes

src/client/common/platform/fileSystemWatcher.ts Show resolved Hide resolved

Update ignore list

3ef70e3

ericsnowcurrently suggested changes Nov 4, 2020

View reviewed changes

Add disposable registry

5925ab1

karrtikr force-pushed the baseclass branch from d65c331 to 183fdce Compare November 5, 2020 00:46

Modify FSWatching Locator

31f7c54

karrtikr force-pushed the baseclass branch from 183fdce to 31f7c54 Compare November 5, 2020 00:48

karrtikr requested a review from ericsnowcurrently November 5, 2020 00:52

karrtikr mentioned this pull request Nov 5, 2020

Fix failing global virtual env watcher tests #14633

Merged

ericsnowcurrently approved these changes Nov 5, 2020

View reviewed changes

Kartik Raj added 3 commits November 5, 2020 15:48

Code reviews

fec8c1b

Use string[]

21b740a

Remove list disposable getter

2260b17

karrtikr merged commit 16d4701 into microsoft:main Nov 6, 2020

karrtikr deleted the baseclass branch November 6, 2020 00:06

	private readonly getRoots: () => Promise<string[]> \| string,
	private readonly getRoots: () => Promise<string[]> \| string[],

Added FSWatching base class and made related changes #14605

Added FSWatching base class and made related changes #14605

Uh oh!

Conversation

karrtikr commented Nov 4, 2020

Uh oh!

codecov-io commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

karrtikr Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karrtikr Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

karrtikr Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Nov 5, 2020

Uh oh!

Uh oh!

codecov-io commented Nov 4, 2020 •

edited

Loading

karrtikr Nov 5, 2020 •

edited

Loading