Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 30 commits into from
Sep 15, 2020
Merged

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

merged 30 commits into from
Sep 15, 2020

Conversation

devabhishekpal
Copy link
Contributor

@devabhishekpal devabhishekpal 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

@devabhishekpal devabhishekpal deleted the provider_5871 branch August 21, 2020 10:13
@devabhishekpal devabhishekpal restored the provider_5871 branch August 21, 2020 10:14
@devabhishekpal
Copy link
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
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.

@devabhishekpal
Copy link
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
Collaborator

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.
 * ------------------------------------------------------------------------------------------ */
```.

@devabhishekpal
Copy link
Contributor Author

Ok sir making changes and committing.

@Colengms
Copy link
Collaborator

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.

@devabhishekpal
Copy link
Contributor Author

I am on it @Colengms

@devabhishekpal
Copy link
Contributor Author

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

@devabhishekpal
Copy link
Contributor Author

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

@devabhishekpal
Copy link
Contributor Author

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

@michelleangela
Copy link
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
@devabhishekpal
Copy link
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
Collaborator

@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
Copy link
Collaborator

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.

@devabhishekpal
Copy link
Contributor Author

Okayyy I will check it

@sean-mcmanus
Copy link
Collaborator

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

@devabhishekpal
Copy link
Contributor Author

Yeah I undated those just now @sean-mcmanus sir

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


Copy link
Collaborator

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
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
Collaborator

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.

@devabhishekpal
Copy link
Contributor Author

Okayy

Updated .eslintrc.js
@devabhishekpal
Copy link
Contributor Author

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

@devabhishekpal
Copy link
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
Collaborator

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

@devabhishekpal
Copy link
Contributor Author

Okayy

@devabhishekpal
Copy link
Contributor Author

devabhishekpal commented Sep 12, 2020

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

@sean-mcmanus
Copy link
Collaborator

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.

@devabhishekpal
Copy link
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