Update the code base to use IFileSystem instead of fs and fs-extra. by ericsnowcurrently · Pull Request #7915 · microsoft/vscode-python

ericsnowcurrently · 2019-10-10T23:19:48Z

(for #6911)

This is a precursor to the actual fix. It ensures that we really do use the new FS API everywhere.

I've left out changes related to datascience. I'll open a separate issue for 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.~~

src/client/interpreter/locators/helpers.ts

codecov-io · 2019-10-21T20:35:49Z

Codecov Report

Merging #7915 into master will decrease coverage by 0.02%.
The diff coverage is 69.09%.

@@            Coverage Diff             @@
##           master    #7915      +/-   ##
==========================================
- Coverage    58.1%   58.07%   -0.03%     
==========================================
  Files         527      526       -1     
  Lines       27075    26827     -248     
  Branches     4043     4040       -3     
==========================================
- Hits        15731    15581     -150     
+ Misses      10485    10389      -96     
+ Partials      859      857       -2

Impacted Files	Coverage Δ
...rc/client/debugger/debugAdapter/serviceRegistry.ts	`0% <ø> (ø)`	⬆️
src/client/sourceMapSupport.ts	`81.25% <ø> (ø)`	⬆️
.../datascience/interactive-common/interactiveBase.ts	`24.79% <0%> (ø)`	⬆️
...rpreter/locators/services/baseVirtualEnvService.ts	`28.3% <0%> (ø)`	⬆️
src/client/datascience/jupyter/kernelService.ts	`55.1% <100%> (ø)`	⬆️
src/client/common/net/fileDownloader.ts	`100% <100%> (ø)`	⬆️
src/client/common/variables/environment.ts	`92.4% <100%> (+0.09%)`	⬆️
src/client/common/platform/serviceRegistry.ts	`100% <100%> (ø)`	⬆️
src/client/common/platform/types.ts	`100% <100%> (ø)`	⬆️
...preter/locators/services/windowsRegistryService.ts	`90% <100%> (ø)`	⬆️
... and 48 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 9411e98...5eb00e1. Read the comment docs.

kimadeline

I have a couple of questions, otherwise it looks good. The intermediate state of fileSystem.ts is a bit 😬, looking forward to seeing its final form :D

src/client/common/editor.ts

src/client/common/envFileParser.ts

src/client/debugger/debugAdapter/main.ts

src/client/interpreter/locators/helpers.ts

src/test/common/platform/filesystem.unit.test.ts

src/test/linters/lint.unit.test.ts

src/client/common/platform/fileSystem.ts

DonJayamanne · 2019-10-28T17:59:44Z

src/client/common/platform/fileSystem.ts


    public async rmtree(dirname: string): Promise<void> {
-        return this.fsExtra.rmdir(dirname);
+        return this.fsExtra.stat(dirname)


Why are we performing a check here.
This doesn't look right. The calling code should not be invoking this method if the directory doesn't exists. I.e. calling code shoudl fall over if its called incorrectly.
Basically we're swallowing exceptions here.

It's because fsextra.remove() does not fail if the directory does not exist. Therefore we have to do the manual check.

How about we to try and delete and swallow exceptions if any.
That's faster than checking file info then deleting. I.e. perform one I/O operation

DonJayamanne · 2019-10-28T18:01:58Z

src/client/common/platform/fileSystem.ts

+
+export class FileSystemPath implements IFileSystemPath {
+    constructor(
+        private readonly isWindows = (getOSType() === OSType.Windows),


Why is this an optional parameter?
Looks like code smell again!

The default value captures the normal use case.

What's the other use case?

DonJayamanne · 2019-10-28T18:03:30Z

src/client/common/platform/fileSystem.ts

+        return this.raw.join(...filenames);
+    }
+
+    public normCase(filename: string): string {


Do we really need this? Is this something VS Code API has added or are we bringing in something from python world in here? I know we're had a tonne of issues with normcase in debugger and pytest adapter, and not looking forward to introducing an unnecessary complication in the node/extension world
@karthiknadig (you know about the norm case issues in debugger better than me)

Checking if it is platform is not sufficient on windows. Individual folders can have case-sensitive settings enabled on them. I would leave this to platform APIs or VS Code itself to figure out.

This covers the logic from existing. All I'm doing is pulling it out into an explicit function. If it's a problem then it's an existing problem. In that case please open a separate issue.

But why create a method for this, when all we need is a way to compare paths and we have a simple meeting already. I.e. when else would norm case be used?

src/client/common/platform/fileSystem.ts

DonJayamanne · 2019-10-28T18:06:57Z

src/client/common/platform/fileSystem.ts

-        private readonly isWindows = (getOSType() === OSType.Windows),
-        //public readonly raw: IFileSystem = {}
-        public readonly raw: IRawFileSystem = new RawFileSystem()
+        public readonly raw: IRawFileSystem = new RawFileSystem(),


Don't see why we are injecting parameters with default values?
This isn't node style code! Seems like this is written specially for testing purposes, These should be injected using DI.

I'm unclear on the problem here. We can talk about the topic more, but I'd rather get this PR merged first.

@DonJayamanne do you have a link on that? I couldn't find relevant answers on the internet 🕵️‍♀️ Personally I'm not a big fan of instantiating objects/complex values as default parameters.

I agree.
This had been discussed with the broader team (including DS) and we agreed this isn't something we'd continue doing.
Ideal we should remove this default parameters, been discussed

Personally I'm not a big fan of instantiating objects/complex values as default parameters

I'll update our standing standards guide (with what was discussed and decided) so there no confusion.

DonJayamanne

Please do not add normcase API/function if not required.
Other comments are optional, though we agreed not to go with default parameters for testing purposes.

karthiknadig · 2019-10-28T18:46:52Z

src/client/common/platform/fileSystem.ts

        }
+        path1 = this.path.normCase(path1);
+        path2 = this.path.normCase(path2);
+        return path1 === path2;


This comparison may be incorrect. What this is checking is if the strings are the same, not really the paths. What happens if the path1 is short-path and path2 is long path to the same file? This should at minimum expand to long path on windows. I would defer this to platform APIs as well. Old implementation was incorrect as well.

Please open a separate issue for this.

we have a way to compare file paths today. This sounds and looks like python style norm casing.
It's called arePathsSame or similar.

I.e. there's never a need to do norm casing, this should be done only when comparing paths and we have a simple generic method for that today.

ericsnowcurrently · 2019-10-28T20:51:59Z

trigger re-test

kimadeline

I have a couple of questions, but I also disagree with the way you've structured your tests: since some of your tests are wrapped in if/else conditions, it means that the test suites are not idempotent since they are not hitting each test no matter the platform and skipping them as needed (i hope i'm using that word correctly 😬).

src/client/common/platform/fileSystem.ts

src/client/interpreter/locators/helpers.ts

src/test/common/variables/envVarsService.unit.test.ts

src/test/interpreters/windowsRegistryService.unit.test.ts

src/test/common/platform/filesystem.functional.test.ts

src/client/common/platform/fileSystem.ts

src/test/common/platform/filesystem.unit.test.ts

DonJayamanne · 2019-11-04T22:41:16Z

src/client/common/platform/types.ts

-    getRealPath(path: string): Promise<string>;
+export interface IFileSystemPath {
+    join(...filenames: string[]): string;
+    normCase(filename: string): string;


This isn't used.

src/client/common/platform/fileSystem.ts

src/test/common/platform/filesystem.functional.test.ts

src/test/common/platform/filesystem.unit.test.ts

src/test/interpreters/windowsRegistryService.unit.test.ts

kimadeline · 2019-11-07T18:53:11Z

Why did the GH VS Code extension publish my comments as single reviews even though they were marked as "pending" (╯°□°)╯︵ ┻━┻

outdated

ericsnowcurrently added the no-changelog No news entry required label Oct 10, 2019

DonJayamanne reviewed Oct 11, 2019

View reviewed changes

src/client/interpreter/locators/helpers.ts Outdated Show resolved Hide resolved

ericsnowcurrently force-pushed the consolidate-fs-usage branch 2 times, most recently from 37906bf to 5aae7b5 Compare October 21, 2019 19:40

ericsnowcurrently requested a review from kimadeline October 21, 2019 21:29

ericsnowcurrently force-pushed the consolidate-fs-usage branch from 0416f37 to 2afbf96 Compare October 22, 2019 21:50

ericsnowcurrently closed this Oct 22, 2019

ericsnowcurrently reopened this Oct 22, 2019

kimadeline suggested changes Oct 23, 2019

View reviewed changes

DonJayamanne reviewed Oct 28, 2019

View reviewed changes

DonJayamanne requested changes Oct 28, 2019

View reviewed changes

karthiknadig reviewed Oct 28, 2019

View reviewed changes

ericsnowcurrently force-pushed the consolidate-fs-usage branch from f43e6b3 to 74a59e8 Compare October 28, 2019 18:48

ericsnowcurrently closed this Oct 28, 2019

ericsnowcurrently reopened this Oct 28, 2019

ericsnowcurrently requested a review from kimadeline October 28, 2019 21:37

kimadeline suggested changes Oct 30, 2019

View reviewed changes

ericsnowcurrently mentioned this pull request Oct 30, 2019

Use new VS Code filesystem API. #8307

Merged

5 tasks

ericsnowcurrently force-pushed the consolidate-fs-usage branch 2 times, most recently from 78fc42e to 83ae768 Compare November 4, 2019 21:34

DonJayamanne previously requested changes Nov 4, 2019

View reviewed changes

kimadeline mentioned this pull request Nov 7, 2019

Consolidate 'crypto' usage #8438

Closed

kimadeline reviewed Nov 7, 2019

View reviewed changes

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

src/client/common/platform/fileSystem.ts Outdated Show resolved Hide resolved

src/client/common/platform/fileSystem.ts Outdated Show resolved Hide resolved

kimadeline reviewed Nov 7, 2019

View reviewed changes

src/test/common/platform/filesystem.functional.test.ts Outdated Show resolved Hide resolved

kimadeline reviewed Nov 7, 2019

View reviewed changes

src/test/common/platform/filesystem.functional.test.ts Outdated Show resolved Hide resolved

kimadeline reviewed Nov 7, 2019

View reviewed changes

src/test/common/platform/filesystem.unit.test.ts Show resolved Hide resolved

kimadeline reviewed Nov 7, 2019

View reviewed changes

src/test/interpreters/windowsRegistryService.unit.test.ts Outdated Show resolved Hide resolved

ericsnowcurrently added 25 commits November 25, 2019 17:23

Use TempFileSystem in FileSystemUtils.isDirReadonly().

d97b96b

Update a log message.

7b5562f

Make test setup more clear.

0100179

Drop a superfluous check in a test.

39f7353

Use the temp dir callback during test cleanup.

030156e

Use tmpDir.removeCallback() during test cleanup.

aa00bc5

Use skips for platform-specific tests.

97d190a

Fix a typo.

7176ec1

Simplify some test setup.

5bb137d

Be more explicit about expected args in a mock.

826a9a2

Fix the tmp FS tests.

abce7e8

Add a test for reading bogus files.

5a023b3

Use assertions for checking if files exist (or don't).

c8cb26f

Add missing doc comments.

74564a7

Drop an unused interface method.

32243b6

Do not provide defaults in the constructors.

1f8e46f

Make inversify happy about the FileSystem class.

0f137f0

Drop dead comments.

784a241

Use util.promisify() for glob().

242c1ee

Drop a dead comment.

e3cf740

Drop some whitespace.

97e5278

Use normal import for chai-as-promised.

bde62a3

Fix how some tests are skipped.

7bbd06a

Drop pathExistsSync().

1dd4d09

Fix typos and clarify comments.

5eb00e1

ericsnowcurrently force-pushed the consolidate-fs-usage branch from 546a075 to 5eb00e1 Compare November 26, 2019 00:36

ericsnowcurrently merged commit a23761d into microsoft:master Nov 26, 2019

ericsnowcurrently deleted the consolidate-fs-usage branch November 26, 2019 01:11

lock bot locked as resolved and limited conversation to collaborators Dec 3, 2019

Conversation

ericsnowcurrently commented Oct 10, 2019

Uh oh!

Uh oh!

codecov-io commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kimadeline 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!

DonJayamanne Oct 28, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Oct 30, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne left a comment • 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!

karthiknadig Oct 28, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently commented Oct 28, 2019

Uh oh!

kimadeline left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Oct 21, 2019 •

edited

Loading

DonJayamanne Oct 28, 2019 •

edited

Loading

DonJayamanne Oct 30, 2019 •

edited

Loading

DonJayamanne left a comment •

edited

Loading

karthiknadig Oct 28, 2019 •

edited

Loading

kimadeline left a comment •

edited

Loading