Skip to content

Use new VS Code filesystem API. #8307

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 25 commits into from
Nov 26, 2019

Conversation

ericsnowcurrently
Copy link

@ericsnowcurrently ericsnowcurrently commented Oct 30, 2019

(for #6911)

This PR builds on #7915, focusing only on the move to the new filesystem API. Note that there are 6 functions (in addition to glob() and tmp.file()) that I was not able to convert due to a lack of functionality in the new API. I have opened upstream issues to address that.

  • 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.

@codecov-io
Copy link

codecov-io commented Nov 4, 2019

Codecov Report

Merging #8307 into master will increase coverage by 0.58%.
The diff coverage is 85.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8307      +/-   ##
==========================================
+ Coverage   58.07%   58.66%   +0.58%     
==========================================
  Files         526      526              
  Lines       26827    27168     +341     
  Branches     4040     4043       +3     
==========================================
+ Hits        15581    15938     +357     
+ Misses      10389    10368      -21     
- Partials      857      862       +5
Impacted Files Coverage Δ
src/client/common/platform/types.ts 100% <ø> (ø) ⬆️
src/client/workspaceSymbols/provider.ts 93.33% <100%> (ø) ⬆️
src/client/common/platform/fileSystem.ts 74.77% <85.71%> (-2.22%) ⬇️
...lient/datascience/jupyter/jupyterSelfCertsError.ts 100% <0%> (ø) ⬆️
src/client/debugger/extension/serviceRegistry.ts 100% <0%> (ø) ⬆️
src/client/common/variables/serviceRegistry.ts 100% <0%> (ø) ⬆️
src/client/common/process/serviceRegistry.ts 100% <0%> (ø) ⬆️
src/client/common/dotnet/serviceRegistry.ts 100% <0%> (ø) ⬆️
src/client/linters/serviceRegistry.ts 100% <0%> (ø) ⬆️
... and 30 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 a23761d...d9e54d2. Read the comment docs.

@kimadeline
Copy link

kimadeline commented Nov 7, 2019

The link you provided says "commit range not found", so I've been using 6f63744 (Fix the tmp FS tests. which is the last commit in #7915) as my baseline: https://github.com/microsoft/vscode-python/pull/8307/files/6f6374441e74d62111008a3dfef0961c9bd43e40..HEAD, and it's not just 6 files changed and 932 insertions:

image

@ericsnowcurrently
Copy link
Author

FYI, the one lingering failure (functional tests) is something I have not been able to reproduce locally yet.

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.

Not sure whether this is still ready for review.
It would be better to split such PRs into smaller chunks, or perhaps we've stopped doing that.
E.g. we could implement the new API in one PR & not use it anywhere, then update other files in another PR, then more i.e. gradually strip out the use of the old API in increments... making it much easier to test and review.
Also, I think it would be easier to get the work done as well, i.e. split amongst others.. Anyways, just a suggestion.
@karthiknadig /cc

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 believe all of my original concerns have been addressed.
I don't wish to hold the PR unnecessarily. hence approving.

@ericsnowcurrently
Copy link
Author

Oh, I absolutely agree about smaller PRs. This one was an aberration due to lack of clarity early on back when the PR was much smaller. :)

@ericsnowcurrently ericsnowcurrently merged commit 7cbd1c2 into microsoft:master Nov 26, 2019
@ericsnowcurrently ericsnowcurrently deleted the use-new-fs-api branch November 26, 2019 18:48
@lock lock bot locked as resolved and limited conversation to collaborators Dec 3, 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.

4 participants