Add setting to control speed of diagnostic publishing + detect old MacOS #2350
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2350 +/- ##
=========================================
- Coverage 75.69% 75.6% -0.09%
=========================================
Files 309 309
Lines 14341 14365 +24
Branches 2540 2546 +6
=========================================
+ Hits 10855 10861 +6
- Misses 3476 3494 +18
Partials 10 10
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks fine, but you might want to alter how versionMajor/Minor are kept.
Also, please reach out to @ericsnowcurrently as he was working on #2245 today as well.
| @@ -54,6 +54,7 @@ export class LanguageServerDownloader { | |||
| let localTempFilePath = ''; | |||
| try { | |||
| localTempFilePath = await this.downloadFile(downloadUri, 'Downloading Microsoft Python Language Server... '); | |||
| // await this.verifyDownload(localTempFilePath, platformString); | |||
There was a problem hiding this comment.
We really should be verifying downloads. This code only temporary disabled to allow hot updates and that new build does not generate engine hashes (it should). Signing LS and nuget is not sufficient without verification.
There was a problem hiding this comment.
Cool. Shouldn't we just add that code when it is enabled then?
There was a problem hiding this comment.
With the change of the build hash generation is gone. Someone needs to port it from my definition to the one Steve made.
| @@ -115,6 +125,15 @@ export class LanguageServerExtensionActivator implements IExtensionActivator { | |||
| (this.configuration.getSettings() as PythonSettings).removeListener('change', this.onSettingsChanged.bind(this)); | |||
| } | |||
|
|
|||
| private checkSupportedPlatform(): boolean { | |||
| const platform = this.services.get<IPlatformService>(IPlatformService); | |||
| if (platform.isMac && platform.versionMajor === 10 && platform.versionMinor < 12) { | |||
There was a problem hiding this comment.
Wouldn't you want to test for platform.versionMajor < 10 as well here?
There was a problem hiding this comment.
I don't think those exist, that would be PowerPC OS 9, right?
There was a problem hiding this comment.
Not sure, I'm just looking at the logic driving this decision tree 😃.
| const parts = this.version.split('.'); | ||
| return parts.length > 0 ? parseInt(parts[0], 10) : 0; | ||
| } | ||
| public get versionMinor(): number { |
There was a problem hiding this comment.
Perhaps these should be strings? What happens with v10.1s?
There was a problem hiding this comment.
Numbers for comparisons. How do we compare random strings? Checked Linux and Windows, they are numbers. Will check on Mac. Possible drop leading 'v'?
There was a problem hiding this comment.
That's fair. However, I like @DonJayamanne's idea about making a version string a type and working from that.
There was a problem hiding this comment.
Individual parts can be numbers only if they are guaranteed to be numeric. If not, then we can cast when performing the necessary comparison.
|
Also missing are some unit/system tests for the supported or unsupported OS. |
src/client/common/platform/types.ts
Outdated
| @@ -27,6 +27,9 @@ export interface IPlatformService { | |||
| is64bit: boolean; | |||
| pathVariableName: 'Path' | 'PATH'; | |||
| virtualEnvBinName: 'bin' | 'scripts'; | |||
| version: string; | |||
There was a problem hiding this comment.
Shouldn't version be it's own type definition?
There was a problem hiding this comment.
This is string returned by os.release() so it has value if some other code wants to parse it differently.
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Why aren't we checking for Windows and Linux?
We need to check minimum requirements on those OS too.
There was a problem hiding this comment.
Windows is trivial but not really necessary since VSC supports 7,8,10 and so does .NET Core. Anyone knows all the Linux version numbers? ;-)
Fixes #2270