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?

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.

See #2421

• Optional parameter should be at the end.
• Why is this optional?

Agreed, I will move it - not sure why I missed that!

• LanguageServerDownloader move 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'.

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 .

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 LanguageServerDownloader class. 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.

See #2421

Just use return

Ok, will do!

• RequestWithProxy::getRequestOptions remove return undefined and make it just return instead.

! is not necessary due to type guards.
Infact you should be able to use return request(uri, requestOptions)

Ok, I'll try that. I believe the linter was complaining about this but I may be mistaken.

• Remove unnecessary ! in downloadFileRequest

I was mistaken. All good.

It provides a service of downloading a file, it does not wrap anything!
Please change the method to downliadFile.

Sure thing.

• Reconsider the names IRequestWrapper (not a wrapper) and downloadFileRequest.

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.

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.

See issue #2421

FWIW, I find it clearer to put the opening brace on its own line:

    private engineFolder: string,
    private requestHandler?: IDownloadFileService
) {

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.

Doesn't this need the following?

export const IDownloadFileService = Symbol('IDownloadFileService');

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.

superfluous blank line

Agreed.

Ha! We weren't even using this.fs already?

This doesn't need readonly, right?

I felt this was appropriate, since I am not changing the state of that object and am only reading from it.

Ah, I didn't realize you could mark parameters as readonly. Cool!

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);
    }

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)

Agreed.

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.

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.

Blech! This async stuff is so gross! Kudos for knowing how to do this. :)

There is also createDeferred() that simplifies things. Rough equivalent of the TaskCompletionSource in C#

I've not learned why/how to use the createDeferred method yet, and was simply following the pattern from the methods above!

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.

You found me out! Lazy me.

• Move tests into appropriate test suites.

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.

Splitting up this line might be more readable:

    requestProgress(
        this.requestHandler!.downloadFile(uri))

Agreed

smart tests!

It worked on Windows as is but OK

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

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.

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.

+1

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:

• https://martinfowler.com/articles/mocksArentStubs.html#TheDifferenceBetweenMocksAndStubs
• https://testing.googleblog.com/2013/07/testing-on-toilet-know-your-test-doubles.html