GeezerVison

View reviewed changes

Comment thread

src/server/protocol.ts Outdated

		errorCodes?: number[];
		}

		export interface GetCombinedCodeFixRequestArgs extends FileRequestArgs {

Mohamed Hegazy (mhegazy) Nov 29, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add a GetCombinedCodeFixResponse definition

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

View reviewed changes

Comment thread

src/server/protocol.ts Outdated

		}

		export interface GetCombinedCodeFixRequestArgs extends FileRequestArgs {
		groupId: {};

Mohamed Hegazy (mhegazy) Nov 29, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why would not this be string? i am assuming to allow for an array, but when would you need this? u have an example in mind?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

View reviewed changes

Comment thread

src/server/protocol.ts Outdated


		export interface CodeActionAll {
		changes: FileCodeEdits[];
		commands: {}[] \| undefined;

Mohamed Hegazy (mhegazy) Nov 29, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why not optional instead?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

View reviewed changes

Comment thread

src/server/protocol.ts Outdated


		export interface CodeFix extends CodeAction {
		/** If present, one may call 'getAllCodeFixesInGroup' with this groupId. */
		groupId: {} \| undefined;

Mohamed Hegazy (mhegazy) Nov 29, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why not optional instead.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

View reviewed changes

Comment thread

src/services/codeFixProvider.ts Outdated

		getCodeActions(context: CodeFixContext): CodeAction[] \| undefined;
		getCodeActions(context: CodeFixContext): CodeFix[] \| undefined;
		groupIds: string[];
		fixAllInGroup(context: CodeFixAllContext): CodeActionAll;

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this should be optional. exsiting users like tslint for instance do not supply this function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

though this si not a public API.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Also a name that still include codeActions would be better.. something that maintains parity with getCodeActions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

maybe getAllCodeActions or getAllCodeActionsInFile

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

View reviewed changes

Comment thread

src/services/codeFixProvider.ts Outdated

		errorCodes: number[];
		getCodeActions(context: CodeFixContext): CodeAction[] \| undefined;
		getCodeActions(context: CodeFixContext): CodeFix[] \| undefined;
		groupIds: string[];

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think a better name is actionIds or actionCodes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

View reviewed changes

Comment thread

src/services/codeFixProvider.ts Outdated

		}

		export interface CodeFixAllContext extends CodeFixContextBase {
		groupId: {};

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

i think we should just make this a string, unless we have a reason not to.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Nov 30, 2017

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If we use string we are limited to treating it like an enum. With an object we could support any data as part of an action id, such as the name of a class if we wanted to limit fixes to just that. That could be done with a string, but we would have to stringify and parse the data, which is silly because we're sending this through JSON already anyway.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

View reviewed changes

Comment thread

src/services/codefixes/disableJsDiagnostics.ts Outdated

		newText: `// @ts-nocheck${newLineCharacter}`
		}])],
		// groupId unnecessary because adding `// @ts-nocheck` even once will ignore every error in the file.
		groupId: undefined,

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

would it be better to split this CodeFix into two, and make sure that a CodeFix is a union type, that either it has fillAllInGroup and returns groupId/groupIds or it does not.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Nov 30, 2017

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think it would be a pain to have to split up a codefix just to allow multiple (or optional) groupIds. If anything I think we should consolidate them more, like fixClassDoesntImplementInheritedAbstractClass and fixClassIncorrectlyImplementsInterface.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) reviewed

View reviewed changes

Andrew Casey (amcasey) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm not done reviewing but I wanted to indicate that I'm looking at this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/server/protocol.ts Outdated

		GetCodeFixesFull = "getCodeFixes-full",
		GetCombinedCodeFix = "getCombinedCodeFix",
		/* @internal */
		GetCombinedCodeFixFull = "getCombinedCodeFix",

Andrew Casey (amcasey) Nov 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"getCombinedCodeFix-full"?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Rename things

be19542

Mohamed Hegazy (mhegazy) reviewed

View reviewed changes

Comment thread

src/server/protocol.ts Outdated

		commands?: {}[];
		}

		export interface CodeActionAll {

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

CombinedCodeActions?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

View reviewed changes

Comment thread

src/services/codeFixProvider.ts Outdated

		getCodeActions(context: CodeFixContext): CodeAction[] \| undefined;
		getCodeActions(context: CodeFixContext): CodeFix[] \| undefined;
		actionIds: string[];
		getAllCodeActions(context: CodeFixAllContext): CodeActionAll;

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

we should make this one optional

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Or make CodeFixRegisraction a union type

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

not all codeFixes need to provide a list of applicable actionIds, and not all of them need to provide a getAllCodeActions feature and not all of them need to return an actionId in their output.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

View reviewed changes

Comment thread

src/services/utilities.ts Outdated

		}

		/** Add a value to a set, and return true if it wasn't already present. */
		export function addToSeenIds(seen: true[], key: number): boolean {

Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why not jsut use a map for these, it is a sparse array anyways..

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andy Hanson added 2 commits

November 30, 2017 15:47


          Code review

8a1865f


          Merge branch 'master' into codeFixAll

b89b7fe

Andrew Casey (amcasey) commented Dec 1, 2017

Copy link

Copy Markdown

Member

We've had some problems in the past when we've tried to make multiple changes to the same range in a single ChangeTracker. Do we have some reason to believe that won't occur when combining code fixes?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) reviewed

Dec 1, 2017

View reviewed changes

Comment thread

src/services/codeFixProvider.ts Outdated


		export function registerCodeFix(codeFix: CodeFix) {
		forEach(codeFix.errorCodes, error => {
		export function registerCodeFix(codeFix: CodeFixRegistration) {

Andrew Casey (amcasey) Dec 1, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Now that there's a CodeFix type, codeFix is a confusing name for a CodeFixRegistration.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) reviewed

Dec 1, 2017

View reviewed changes

Andrew Casey (amcasey) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is this based on Roslyn's implementation? Using theirs as a model might help us avoid making the same mistakes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/types.ts Outdated


		export interface CodeFix extends CodeAction {
		/** If present, one may call 'getCombinedCodeFix' with this actionId. */
		actionId?: {} \| undefined;

Andrew Casey (amcasey) Dec 1, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is on a separate type for back compat reasons? Making it optional isn't enough?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Dec 1, 2017

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A CodeFix includes both a CodeAction and an optional action id. A CodeAction will only have an actionId if returned from a code fix, not if from a completion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/codeFixProvider.ts Outdated

		});
		}
		if (codeFix.actionIds) {
		for (const gid of codeFix.actionIds) {

Andrew Casey (amcasey) Dec 1, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

gid is out of date

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Rename things

a8b3afc

Deleted user (ghost) commented Dec 1, 2017

Copy link

Copy Markdown

Author

We've had some problems in the past when we've tried to make multiple changes to the same range in a single ChangeTracker. Do we have some reason to believe that won't occur when combining code fixes?

We know that ChangeTracker can't handle all possible sequences of changes because some would be nonsensical -- so it's a codefix's responsibility to ensure it's doing the possible and not e.g. making any changes twice (see the many addToSeen calls).
Since ChangeTracker already has the responsibility of making multiple changes, I think it's best to make all changes through there and make improvements to ChangeTracker if any problems come up. (
There was one problem that came up when deleting the last and next-to-last items in a list, fixed in this PR.) That should be easier than trying to combine raw text changes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Update API baselines

a38216c

Andrew Casey (amcasey) commented Dec 1, 2017

Copy link

Copy Markdown

Member

I don't want to bikeshed on naming, but it seems strange that a CodeFix is a CodeAction with an (optional) actionId. If the ID is for the action, why isn't it on the action? Maybe a CodeFixAction is a CodeAction with a fixId? Mohamed Hegazy (@mhegazy)?

mhegazy reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) commented Dec 1, 2017

Copy link

Copy Markdown

Member

I'm not quite clear on the difference between a CombinedCodeAction and a CodeActionAll. Do we just use different names in different layers?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          CodeActionAll -> CombinedCodeActions

c621e3f

Mohamed Hegazy (mhegazy) commented Dec 2, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

This is the extra ID I was thinking of:
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/CodeActions/CodeAction.cs,48

so we should generate that from all the inputs to the codefix to make sure they are unique?

but what to do if combining the fixes is not just applying them in sequence?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) commented Dec 2, 2017

Copy link

Copy Markdown

Member

Mohamed Hegazy (@mhegazy) I think it's more or less equivalent to what we're calling action/fix ID and we have the option of incorporating the code in the future if that turns out to be interesting.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andy Hanson added 4 commits

December 5, 2017 10:12


          Merge branch 'master' into codeFixAll

d0af16a


          Take a scope parameter instead of fileName for flexibility

2db8649


          Renames and bugfixes

f2031be


          Merge branch 'master' into codeFixAll

79c0150

Andrew Casey (amcasey) commented Dec 6, 2017

Copy link

Copy Markdown

Member

Does VS offer all levels of repetition for all code fixes or do code fixes somehow indicate that they can be repeated? If they indicate that they can be repeated, do they somehow indicate which choices would be sensible? (e.g. don't offer 'project' if all occurrences are in 'file')

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) commented Dec 6, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Roslyn appears to offer all scopes when repetition is offered, but not always offer repetition (e.g. add missing member), so we might need to add a bit to CodeFixAction. Or would you just omit the fixId in that case?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ghost mentioned this pull request

Make getCombinedCodeFix public #20538

Closed


          Make API changes internal

8a61761

ghost force-pushed the codeFixAll branch from fce0913 to 8a61761 Compare

December 7, 2017 15:23


          Merge branch 'master' into codeFixAll

41c5710

Deleted user (ghost) commented Dec 7, 2017

Copy link

Copy Markdown

Author

Andrew Casey (@amcasey) Mohamed Hegazy (@mhegazy) I've made the API changes internal for now (and marked with #20538). I'd like to get this PR in soon since it touches a lot of files.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sheetal Nandi (sheetalkamat) reviewed

View reviewed changes

Comment thread

src/server/client.ts Outdated

		}

		getCodeFixesAtPosition(file: string, start: number, end: number, errorCodes: number[]): CodeAction[] {
		getCodeFixesAtPosition(file: string, start: number, end: number, errorCodes: number[]): CodeFixAction[] {

Sheetal Nandi (sheetalkamat) Dec 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can the return type be ReadonlyArray ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Dec 7, 2017

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sheetal Nandi (sheetalkamat) reviewed

View reviewed changes

Comment thread

src/harness/fourslash.ts

		const { changes, commands } = this.languageService.getCombinedCodeFix({ type: "file", fileName: this.activeFile.fileName }, fixId, this.formatCodeSettings);
		assert.deepEqual(commands, options.commands);
		this.applyChanges(changes);
		this.verifyCurrentFileContent(newFileContent);

Sheetal Nandi (sheetalkamat) Dec 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

will this always be currentFile. Api call returns TextChangeRange which has file name per change.. So wouldnt it need to verify all those files?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Dec 7, 2017

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Added an assertion that all changes are for the current file and a TODO for if we need to test changes affecting multiple files.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) commented Dec 7, 2017

Copy link

Copy Markdown

Member

Andy (Andrewkraft) (@Andy-MS) Do we have a way to signal that some code fixes can't be repeated? Or, at least, can we add that later without affecting back-compat? Otherwise, LGTM.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Code review

87d30c0

ghost force-pushed the codeFixAll branch from 4df09b0 to 87d30c0 Compare

December 7, 2017 18:26

Deleted user (ghost) commented Dec 7, 2017

Copy link

Copy Markdown

Author

I think we can do that by just not supplying a fixId.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) commented Dec 7, 2017

Copy link

Copy Markdown

Member

Can you please add a comment to that effect on the declaration? I'm worried that future implementers (i.e. me) might forget that that's an option and try to add a flag.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) approved these changes

View reviewed changes


          Update comment

5f7ba73

ghost merged commit 19ea189 into master