Repository navigation
Fix the return value for FileSystem.isReadOnly(). - #9800
Eric Snow (ericsnowcurrently) merged 2 commits into
Conversation
|
BTW, I've already started a CI job for this PR. 😄 |
Codecov Report
@@ Coverage Diff @@
## master #9800 +/- ##
==========================================
- Coverage 60.87% 60.86% -0.01%
==========================================
Files 556 556
Lines 29996 29998 +2
Branches 4547 4547
==========================================
- Hits 18259 18258 -1
- Misses 10710 10713 +3
Partials 1027 1027
Continue to review full report at Codecov.
|
| } else if (settings.globalModuleInstallation) { | ||
| const fs = this.serviceContainer.get<IFileSystem>(IFileSystem); | ||
| if (await fs.isDirReadonly(path.dirname(pythonPath))) { | ||
| if (await fs.isDirReadonly(path.dirname(pythonPath)).catch(_err => true)) { |
There was a problem hiding this comment.
Tests for this?
|
Kartik Raj (@karrtikr), otherwise okay? |
|
CI passed, BTW |
|
Kudos, SonarCloud Quality Gate passed!
|
Don Jayamanne (DonJayamanne)
left a comment
Don Jayamanne (DonJayamanne)
left a comment
There was a problem hiding this comment.
I didn't submit my comments
| throw err; // re-throw | ||
| } | ||
| await fs.close(fd); | ||
| await fs.unlink(filePath); |
There was a problem hiding this comment.
These two could probably, be done in the back ground rather than awaiting.
Basically previously there was no await on cleaning resources, now there is, hence it's unnecessarily slower.
Suggestion is to use fs.close().ignorewait
There was a problem hiding this comment.
Eric Snow (@ericsnowcurrently) Do you think it would be good to create a seprate PR to clean up the resources in the background, as opposed to awaiting on them and slowing the exit from this fn?
There was a problem hiding this comment.
I can submit one if you think its ok.
There was a problem hiding this comment.
That would be great. Thanks!
4 participants
(for #8995)
The return value didn't get fixed when we changed
isPathWritableAsync()toFileSystem.isDirReadonly(). Here we fix that and update the test. We now also fail for any errors except errno EACCES. The one consumer ofisDirReadonly()has been adjusted accordingly, maintaining its does-not-fail behavior.