Skip to content

Add CachingLocator. #14020

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

Conversation

ericsnowcurrently
Copy link

@ericsnowcurrently ericsnowcurrently commented Sep 21, 2020

Note that I factored out the BackgroundRequestLooper class to keep CachingLocator focused, especially as I found it was important to deal with one "refresh" at a time.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • [ ] Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • [ ] Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • [ ] Test plan is updated as appropriate.
  • [ ] package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • [ ] The wiki is updated with any design decisions/details.

@ericsnowcurrently ericsnowcurrently added the no-changelog No news entry required label Sep 21, 2020
@ericsnowcurrently ericsnowcurrently force-pushed the pyenvs-component-caching-locator branch from 390f221 to 48445b7 Compare September 21, 2020 23:17
@ericsnowcurrently ericsnowcurrently force-pushed the pyenvs-component-caching-locator branch from 48445b7 to 85b2b6b Compare September 23, 2020 03:33
@kimadeline kimadeline mentioned this pull request Sep 23, 2020
10 tasks
@ericsnowcurrently ericsnowcurrently force-pushed the pyenvs-component-caching-locator branch from 85b2b6b to e41ccef Compare September 29, 2020 00:23
@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2020

Codecov Report

Merging #14020 into main will increase coverage by 0.07%.
The diff coverage is 64.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #14020      +/-   ##
==========================================
+ Coverage   59.96%   60.04%   +0.07%     
==========================================
  Files         706      710       +4     
  Lines       39180    39420     +240     
  Branches     5681     5708      +27     
==========================================
+ Hits        23494    23668     +174     
- Misses      14449    14501      +52     
- Partials     1237     1251      +14     
Impacted Files Coverage Δ
src/client/pythonEnvironments/index.ts 17.64% <21.42%> (+0.57%) ⬆️
...ronments/base/locators/composite/cachingLocator.ts 64.88% <64.88%> (ø)
src/client/pythonEnvironments/base/envsCache.ts 92.59% <100.00%> (+8.59%) ⬆️
...onments/base/locators/composite/reducingLocator.ts 100.00% <100.00%> (ø)
src/client/common/utils/platform.ts 68.00% <0.00%> (-4.00%) ⬇️
...discovery/locators/services/windowsStoreLocator.ts 100.00% <0.00%> (ø)
src/client/pythonEnvironments/common/posixUtils.ts 91.66% <0.00%> (ø)
...covery/locators/services/posixKnownPathsLocator.ts 100.00% <0.00%> (ø)
... and 2 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 ef44f29...abf86c3. Read the comment docs.

@ericsnowcurrently ericsnowcurrently force-pushed the pyenvs-component-caching-locator branch from e41ccef to 5fabd2a Compare September 29, 2020 02:01
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review September 29, 2020 02:01
@ericsnowcurrently
Copy link
Author

@kimadeline and @karrtikr, I should have left this as a draft PR. I'll let you know when I've wrapped up the last few changes I have to make.

@ericsnowcurrently ericsnowcurrently marked this pull request as draft September 29, 2020 15:41
@kimadeline kimadeline mentioned this pull request Sep 30, 2020
10 tasks
@ericsnowcurrently ericsnowcurrently force-pushed the pyenvs-component-caching-locator branch from 5fabd2a to 0d8f49d Compare September 30, 2020 21:05
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review September 30, 2020 22:25
@ericsnowcurrently
Copy link
Author

@kimadeline, @karrtikr, the PR is ready to go now.

@ericsnowcurrently ericsnowcurrently force-pushed the pyenvs-component-caching-locator branch from 0d8f49d to 9e97735 Compare September 30, 2020 22:40
@kimadeline
Copy link

Since Kartik is out, you may want to request a review from @karthiknadig, because I don't want to shoulder the responsibility of reviewing this PR alone 😆

@karrtikr
Copy link

karrtikr commented Oct 1, 2020

@ericsnowcurrently @kimadeline I just had one comment regarding potential correctness #14020 (comment) I leave the rest upto you guys😄

@karrtikr karrtikr removed their request for review October 1, 2020 19:29
@ericsnowcurrently ericsnowcurrently force-pushed the pyenvs-component-caching-locator branch from 9e97735 to 4cc412e Compare October 5, 2020 20:31
@kimadeline
Copy link

Will re-review this afternoon.

@ericsnowcurrently
Copy link
Author

resetting "checks" trigger

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 8, 2020

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
0.0% 0.0% Duplication

@codecov-io
Copy link

Codecov Report

Merging #14020 into main will decrease coverage by 0.00%.
The diff coverage is 64.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #14020      +/-   ##
==========================================
- Coverage   59.91%   59.91%   -0.01%     
==========================================
  Files         709      712       +3     
  Lines       39334    39520     +186     
  Branches     5698     5722      +24     
==========================================
+ Hits        23567    23678     +111     
- Misses      14529    14586      +57     
- Partials     1238     1256      +18     
Impacted Files Coverage Δ
src/client/pythonEnvironments/index.ts 17.64% <21.42%> (+0.57%) ⬆️
src/client/common/utils/backgroundLoop.ts 60.71% <60.71%> (ø)
...ronments/base/locators/composite/cachingLocator.ts 70.45% <70.45%> (ø)
src/client/pythonEnvironments/base/envsCache.ts 85.18% <84.61%> (+1.18%) ⬆️
...onments/base/locators/composite/reducingLocator.ts 100.00% <100.00%> (ø)
...very/locators/services/virtualenvwrapperLocator.ts 76.92% <0.00%> (-15.39%) ⬇️
src/client/common/utils/platform.ts 60.00% <0.00%> (-12.00%) ⬇️
.../pythonEnvironments/common/externalDependencies.ts 64.10% <0.00%> (-2.57%) ⬇️
src/client/linters/pydocstyle.ts 86.66% <0.00%> (-2.23%) ⬇️
src/client/datascience/debugLocationTracker.ts 76.56% <0.00%> (-1.57%) ⬇️
... and 5 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 fee7e74...fa63318. Read the comment docs.

@ericsnowcurrently ericsnowcurrently merged commit bc1d780 into microsoft:main Oct 8, 2020
@ericsnowcurrently ericsnowcurrently deleted the pyenvs-component-caching-locator branch October 8, 2020 19:54
luabud pushed a commit to luabud/vscode-python that referenced this pull request Oct 26, 2020
Note that we factored out the BackgroundRequestLooper class to keep CachingLocator focused, especially as it was important to deal with one "refresh" at a time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants