Skip to content

Refactored provider classes from client.ts to providers.ts#5998

Merged
sean-mcmanus merged 30 commits intomicrosoft:masterfrom
spacemonkd:provider_5871
Sep 15, 2020
Merged

Refactored provider classes from client.ts to providers.ts#5998
sean-mcmanus merged 30 commits intomicrosoft:masterfrom
spacemonkd:provider_5871

Conversation

@spacemonkd
Copy link
Copy Markdown
Contributor

@spacemonkd spacemonkd commented Aug 21, 2020

Refactored Provider classes ( FoldingRangeProvider, SemanticTokensProvider, DocumentFormattingEditProvider, DocumentRangeFormattingEditProvider, OnTypeFormattingEditProvider ) from client.ts to providers.ts.

Pull request for issue: #5871

Kindly review and suggest necessary changes @bobbrow @Colengms @michelleangela

@spacemonkd spacemonkd closed this Aug 21, 2020
@spacemonkd spacemonkd deleted the provider_5871 branch August 21, 2020 10:13
@spacemonkd spacemonkd restored the provider_5871 branch August 21, 2020 10:14
@spacemonkd spacemonkd reopened this Aug 21, 2020
@spacemonkd
Copy link
Copy Markdown
Contributor Author

Azure pipelines giving missing file header (tslint:file-header) @typescript-eslint/tslint/config error. Any suggestion as to why this is happening?
@bobbrow @Colengms @michelleangela

@bobbrow
Copy link
Copy Markdown
Member

bobbrow commented Aug 21, 2020

Hi @devabhishekpal,

Thanks for taking a look at this issue. It was assigned to @Colengms as I think he had some ideas as to how he wanted it structured. I will defer to him as to whether this is the direction he had in mind.

@spacemonkd
Copy link
Copy Markdown
Contributor Author

Hi @devabhishekpal,

Thanks for taking a look at this issue. It was assigned to @Colengms as I think he had some ideas as to how he wanted it structured. I will defer to him as to whether this is the direction he had in mind.

Thank you dear sir.

@sean-mcmanus
Copy link
Copy Markdown
Contributor

Azure pipelines giving missing file header (tslint:file-header) @typescript-eslint/tslint/config error. Any suggestion as to why this is happening?
@bobbrow @Colengms @michelleangela

I think that error is due to missing the following at the top of provider.ts

/* --------------------------------------------------------------------------------------------
 * Copyright (c) Microsoft Corporation. All Rights Reserved.
 * See 'LICENSE' in the project root for license information.
 * ------------------------------------------------------------------------------------------ */
```.

@spacemonkd
Copy link
Copy Markdown
Contributor Author

Ok sir making changes and committing.

@Colengms
Copy link
Copy Markdown
Contributor

Hi @devabhishekpal . Thanks a lot for jumping in and helping us out. :) The only thing I would have done differently is create a separate file for each provider. We'd also like to move DocumentSymbolProvider, WorkspaceSymbolProvider, FindAllReferencesProvider, and RenameProvider as well. Do you want to tackle these also? :) If not, we could use what is here, and deal with the rest later.

@spacemonkd
Copy link
Copy Markdown
Contributor Author

I am on it @Colengms

@spacemonkd
Copy link
Copy Markdown
Contributor Author

spacemonkd commented Aug 22, 2020

Could you check up on the variables? @Colengms
I created classes and exported them making the variables public static so that we can refernce the same value across file.

Not sure if that is desirable so could you kindly check on it.

export class AbortRequestIdHolder{
abortRequestId;
}

export class ReferenceParamsHolder{
referencesPendingCancellations;
referencesParams;
referencesRequestPending;
}

andd

export class RenameParamsHolder{
renameRequestsPending;
renamePending;
}

these were originally declared as const values.

@spacemonkd
Copy link
Copy Markdown
Contributor Author

Can you please check the commits if possible 😅 @bobbrow @Colengms

@spacemonkd
Copy link
Copy Markdown
Contributor Author

Can someone check the updated code and give a review or changes needed? @bobbrow @sean-mcmanus @Colengms @Colengms @michelleangela

@michelleangela
Copy link
Copy Markdown
Contributor

@devabhishekpal
Thank you for the update. Please note that the team may not be able to review the changes until time permits and based on other higher priority issues and tasks that are also currently being worked on.

@michelleangela michelleangela added this to the 0.30.0 milestone Aug 24, 2020
@spacemonkd
Copy link
Copy Markdown
Contributor Author

@michelleangela thank you for informing. It is alright. As time permits any review of the code would be really helpful, and thanks once again😊

@Colengms
Copy link
Copy Markdown
Contributor

@devabhishekpal Instead of 'Holder' classes, could you make these individually exported fields for now? We eventually need to reorganize those vars. Also, could you move all providers into a new providers subdirectory?

@sean-mcmanus sean-mcmanus requested review from a team and Colengms September 8, 2020 19:15
@sean-mcmanus
Copy link
Copy Markdown
Contributor

sean-mcmanus commented Sep 8, 2020

@devabhishekpal There's a build error on line 1360 of client.ts -- looks like DefaultClient.renamePending should be used. I resolved a merge conflict...not sure if that caused it.

@spacemonkd
Copy link
Copy Markdown
Contributor Author

Okayyy I will check it

@sean-mcmanus
Copy link
Copy Markdown
Contributor

There are some linter issues:
/home/vsts/work/1/s/Extension/src/LanguageServer/client.ts
2618:1 error Expected indentation of 16 spaces but found 14 @typescript-eslint/indent
2619:1 error Expected indentation of 16 spaces but found 14 @typescript-eslint/indent
2619:33 error Unexpected trailing comma comma-dangle

@spacemonkd
Copy link
Copy Markdown
Contributor Author

Yeah I undated those just now @sean-mcmanus sir

import { CppSettings } from '../settings';
import * as editorConfig from 'editorconfig';


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a double newline here and in a few other files. If you feel like it, you could change the existing "no-multiple-empty-lines" in .eslintrc.js to be "no-multiple-empty-lines": ["error", { "max": 1, "maxEOF": 1, "maxBOF": 0}].
If not, we could do that in a separate PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sir

@sean-mcmanus
Copy link
Copy Markdown
Contributor

sean-mcmanus commented Sep 9, 2020

FYI, we're going to delay checking this in until we can merge to release for 0.30.0-insiders5 and 1.0.0...the merge might be in a few hours though...or maybe tomorrow.

@spacemonkd
Copy link
Copy Markdown
Contributor Author

Okayy

Updated .eslintrc.js
@spacemonkd
Copy link
Copy Markdown
Contributor Author

I changed the eslint rules. I will fix the multiple spaces on other files by evening

@spacemonkd
Copy link
Copy Markdown
Contributor Author

Sorry about so many commits...I was editing from my phone so I had to edit each file individually... I have fixed the errors....Kindly check the same.. @sean-mcmanus sir

@sean-mcmanus
Copy link
Copy Markdown
Contributor

Sorry about so many commits...I was editing from my phone so I had to edit each file individually... I have fixed the errors....Kindly check the same.. @sean-mcmanus sir

Cool, thanks. We might be able to check this in tomorrow after we merge for 1.0 (not sure if @Colengms had any more review feedback).

@spacemonkd
Copy link
Copy Markdown
Contributor Author

Okayy

@spacemonkd
Copy link
Copy Markdown
Contributor Author

spacemonkd commented Sep 12, 2020

Is there any other issue for me to look at? @sean-mcmanus sir

@sean-mcmanus
Copy link
Copy Markdown
Contributor

Is there any other issue for me to look at? @sean-mcmanus sir

No issues. We can probably check it in Monday. We've been busy preparing our 1.0 release.

@spacemonkd
Copy link
Copy Markdown
Contributor Author

Okayy sir

@sean-mcmanus sean-mcmanus merged commit 27b5fac into microsoft:master Sep 15, 2020
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants