Allow for the use of a proxy server during Python Language Server download - #2418
Conversation
| this.platform = this.services.get<IPlatformService>(IPlatformService); | ||
| this.platformData = new PlatformData(this.platform, this.fs); | ||
| constructor( | ||
| private readonly output: IOutputChannel, |
There was a problem hiding this comment.
Why are we changing the di pattern? I thought we (you and i )agreed to keep using a simple parameter and user the service locator pattern?
Has anything changed since, to change your decision?
There was a problem hiding this comment.
This particular class was not using DI, so I didn't add it. There was no new decision here, that was all there was to it 😄.
I felt that there was no need to add it to this particular class, seemed unnecessary for what it does. But I am certainly willing to have that discussion!
Should we want to expand this class's design, perhaps that's a different issue deserving of a longer conversation.
| private readonly fs: IFileSystem, | ||
| private readonly platformData: PlatformData, | ||
| readonly workspace: IWorkspaceService, | ||
| private requestHandler: IRequestWrapper | undefined, |
There was a problem hiding this comment.
- Optional parameter should be at the end.
- Why is this optional?
There was a problem hiding this comment.
Agreed, I will move it - not sure why I missed that!
-
LanguageServerDownloadermove optional arguments to the end.
Optional in the sense that you can supply your own implementation only. If you do not supply one, one is supplied for you. Allows us to test the implementation of the request 'wrapper'.
| } | ||
| // Set file to executable (nothing happens in Windows, as chmod has no definition there) | ||
| const executablePath = path.join(installFolder, this.platformData.getEngineExecutableName()); | ||
| fileSystem.chmodSync(executablePath, '0764'); // -rwxrw-r-- |
There was a problem hiding this comment.
Please use an async version, i.e. always use async instead of sync. This is what made node.js popular and faster. We need to remember that and not write old.net style sync code .
There was a problem hiding this comment.
Again, just using what was there before. I can improve on the code of course, but this change was supposed to be a simple one.
I will add an issue to redesign the downloader class to make it more generic/improve the design.
-
Create a new issue to track the redesign of the
LanguageServerDownloaderclass. We should improve upon its design by adding DI, removing any synchronous calls, and making it generally more robust for other use cases. -
Make the use of chmod async. This is the right thing to do.
| proxy: this.proxyUri | ||
| }; | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
Ok, will do!
-
RequestWithProxy::getRequestOptionsremovereturn undefinedand make it justreturninstead.
| public downloadFileRequest(uri: string): request.Request { | ||
| const requestOptions = this.getRequestOptions(); | ||
| if (requestOptions) { | ||
| return request(uri, requestOptions!); |
There was a problem hiding this comment.
! is not necessary due to type guards.
Infact you should be able to use return request(uri, requestOptions)
There was a problem hiding this comment.
Ok, I'll try that. I believe the linter was complaining about this but I may be mistaken.
- Remove unnecessary
!indownloadFileRequest
There was a problem hiding this comment.
I was mistaken. All good.
| } | ||
|
|
||
| export interface IRequestWrapper { | ||
| downloadFileRequest(uri: string): RequestResult; |
There was a problem hiding this comment.
It provides a service of downloading a file, it does not wrap anything!
Please change the method to downliadFile.
There was a problem hiding this comment.
Sure thing.
- Reconsider the names
IRequestWrapper(not a wrapper) anddownloadFileRequest.
| private engineFolder: string) { | ||
|
|
||
| if (!this.requestHandler) { | ||
| this.requestHandler = new RequestWithProxy(workspace.getConfiguration('http').get('proxy', '')); |
There was a problem hiding this comment.
This shouldn't be passed in as an argument. Assume you have to use this class in 10 places, now you have to read the proxy and pass it in 10 places.
It should be accessed by the downloader in the class itself.
Please inject this via DI, we shouldn't be manually constructing concrete classes, goes against IOC pattern.
There was a problem hiding this comment.
Assume you have to use this class in 10 places
We use it in 1, and I didn't think this issue deserved the time to make it more generic. I feel that is a different issue.
now you have to read the proxy and pass it in 10 places
It felt odd coupling the RequestHandler to the WorkspaceService. The downloader doesn't care that you have a Workspace object, it only cares that you have a proxy setting (or not).
Please inject this via DI
...not this change. Let's push that to another.
526be81 to
c7e60b0
Compare
|
Don Jayamanne (@DonJayamanne) please re-review. |
Codecov Report
@@ Coverage Diff @@
## master #2418 +/- ##
==========================================
- Coverage 75.63% 75.14% -0.49%
==========================================
Files 310 310
Lines 14511 14352 -159
Branches 2567 2540 -27
==========================================
- Hits 10975 10785 -190
- Misses 3528 3558 +30
- Partials 8 9 +1
Continue to review full report at Codecov.
|
Eric Snow (ericsnowcurrently)
left a comment
Eric Snow (ericsnowcurrently)
left a comment
There was a problem hiding this comment.
At a high level, LGTM. However there are a bunch of little things I noticed that might be worth cleaning up. Some of them are nits, but some should probably be addressed.
| private readonly platformData: PlatformData, | ||
| readonly workspace: IWorkspaceService, | ||
| private engineFolder: string, | ||
| private requestHandler?: IDownloadFileService) { |
There was a problem hiding this comment.
FWIW, I find it clearer to put the opening brace on its own line:
private engineFolder: string,
private requestHandler?: IDownloadFileService
) {There was a problem hiding this comment.
I think either way is good, and I have tried to follow what I see across the rest of the classes in the extension thus far.
Personally, I like the:
constructor
(
some: string,
params: number,
here: object
)
{
some_code_here();
}...way of doing things, but I'm learning to adapt.
| deactivate(): Promise<void>; | ||
| } | ||
|
|
||
| export interface IDownloadFileService { |
There was a problem hiding this comment.
Doesn't this need the following?
export const IDownloadFileService = Symbol('IDownloadFileService');There was a problem hiding this comment.
Our use of injectify makes use of the Symbol type to identify implementations of interfaces. Since we don't inject the IDownloadFileService into DI (yet) it doesn't need the Symbol definition.
|
|
||
| let localTempFilePath = ''; | ||
| try { | ||
|
|
There was a problem hiding this comment.
superfluous blank line
|
|
||
| const deferred = createDeferred(); | ||
| const fileStream = fileSystem.createWriteStream(tempFile.filePath); | ||
| const fileStream = this.fs.createWriteStream(tempFile.filePath); |
There was a problem hiding this comment.
Ha! We weren't even using this.fs already?
| private readonly output: IOutputChannel, | ||
| private readonly fs: IFileSystem, | ||
| private readonly platformData: PlatformData, | ||
| readonly workspace: IWorkspaceService, |
There was a problem hiding this comment.
This doesn't need readonly, right?
There was a problem hiding this comment.
I felt this was appropriate, since I am not changing the state of that object and am only reading from it.
There was a problem hiding this comment.
Ah, I didn't realize you could mark parameters as readonly. Cool!
| if (requestOptions) { | ||
| return request(uri, requestOptions); | ||
| } | ||
| return request(uri); |
There was a problem hiding this comment.
FWIW, if both cases terminate the function then it's often clearer to use an else:
if (requestOptions) {
return request(uri, requestOptions);
} else {
return request(uri);
}
There was a problem hiding this comment.
6 or 1/2 dozen.
Personally I find the current approach more readable, less nesting. I like an else for such simple statements only when using IIF (immediate ifs)
There was a problem hiding this comment.
Wow Don Jayamanne (@DonJayamanne) jumped in between Eric Snow (@ericsnowcurrently)'s comments and my responses. Fun timez.
I am going with Eric's suggestion here, I likey.
There was a problem hiding this comment.
I definitely agree with Don in cases where there's a clear common path. I try to keep those less indented than the code for the "exceptional" cases. However, when they are equally likely or equally relevant I prefer to keep indentation the same. It helps inform the reader of the nature of the cases.
That said, if either case is more than a few lines it often makes sense to factor it out into a separate private function/method, so that the logic of the original function isn't obscured by handling of the different cases.
| } | ||
|
|
||
| public chmod(filePath: string, mode: string): Promise<void> { | ||
| return new Promise<void>((resolve, reject) => { |
There was a problem hiding this comment.
Blech! This async stuff is so gross! Kudos for knowing how to do this. :)
There was a problem hiding this comment.
There is also createDeferred() that simplifies things. Rough equivalent of the TaskCompletionSource in C#
There was a problem hiding this comment.
I've not learned why/how to use the createDeferred method yet, and was simply following the pattern from the methods above!
| const link = languageServerDownloader.getDownloadUri(); | ||
| assert.equal(link, DownloadLinks[PlatformName.Linux64Bit]); | ||
| }); | ||
| test('Supports download via proxy', async () => { |
There was a problem hiding this comment.
Maybe I'm missing something, but languageServerDownloader isn't used in this test. So doesn't the test belong in src/test/activation/requestWithProxy.unit.test.ts (or at least in its own suite)?
Also, there probably should be separate tests for LanguageServerDownloader.downloadLanguageServer() that use different languageServerDownloader.requestHandler values.
There was a problem hiding this comment.
You found me out! Lazy me.
- Move tests into appropriate test suites.
There was a problem hiding this comment.
Let's ensure we add far more robust testing as part of #2421, as it will need a bit of fine-tuning to make it more accessible to tests.
| }, (progress) => { | ||
|
|
||
| requestProgress(request(uri)) | ||
| requestProgress(this.requestHandler!.downloadFile(uri)) |
There was a problem hiding this comment.
Splitting up this line might be more readable:
requestProgress(
this.requestHandler!.downloadFile(uri))| const fileName = files[0].replace(/\\/g, '/'); | ||
| expect(fileName).to.equal(expectedFileName); | ||
| }); | ||
| test('Ensure creating a temporary file results in a unique temp file path', async () => { |
| fileSystem.chmodSync(executablePath, '0764'); // -rwxrw-r-- | ||
| } | ||
| // Set file to executable (nothing happens in Windows, as chmod has no definition there) | ||
| const executablePath = path.join(installFolder, this.platformData.getEngineExecutableName()); |
There was a problem hiding this comment.
It worked on Windows as is but OK
There was a problem hiding this comment.
I took the opportunity to decouple this class from the IPlatformService (I know we still use PlatformData to get a hold of some names, but those two classes have separate use cases).
|
Eric Snow (@ericsnowcurrently), Don Jayamanne (@DonJayamanne): All PR comments are addressed, if someone could find the time to re-review? Thanks! |
|
Retry the CI on VSTS - uncertain why this would fail! (Cannot reproduce it on Ubuntu linux...) |
Eric Snow (ericsnowcurrently)
left a comment
Eric Snow (ericsnowcurrently)
left a comment
There was a problem hiding this comment.
LGTM
I left a few comments, but they're not that big a deal.
| this.fs, | ||
| this.platformData, | ||
| this.workspace, | ||
| new RequestWithProxy(this.workspace.getConfiguration('http').get('proxy', '')), |
There was a problem hiding this comment.
Mashing this into one line is okay if the risk of exceptions from getConfiguration() and get('proxy') are relatively nil (which AFAICS they are). Otherwise stack traces are a little harder to follow. I'll also argue that this is a bit less readable. :)
I'll leave it up to you.
| proxy: this.proxyUri | ||
| }; | ||
| } else { | ||
| return; |
There was a problem hiding this comment.
I though you were going with return {};. It's fine either way. Mostly this syntax (mixed bare return and non-bare return in the same function) weirds me out! that and the whole return-CoreOptions-OR-undefined makes the function feel much more complex than it actually is.
That said, it's not a huge deal. :) I'll leave this up to you.
| } | ||
| return request(uri); | ||
| const requestOptions: request.CoreOptions | undefined = this.requestOptions; | ||
| return request(uri, requestOptions); |
| return edits ? edits.slice() : undefined; | ||
| } | ||
|
|
||
| createFile(uri: vscUri.URI, options?: { overwrite?: boolean, ignoreIfExists?: boolean }): void { |
There was a problem hiding this comment.
Whoa! I didn't realize we had any pre-built testing doubles! (This is more of a fake than a mock, BTW.) I thought we only use mocks.
See:
NOTE: revert launch.json before submitting a PR
- Provide a default interface that will be used in production - Swap out the interface for testing
- Update downloader to use a request interface to handle a request - implement the interface such that it knows about proxy uris - test that proxy info is handled in the request interface
- give it's download method a more apt name
- decouple LanguageServerDownloader from the workspace service
0ff58c0 to
26fbd00
Compare
All PR comments have been addressed or forwarded to #2421.
4 participants
Fixes #2385