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

Added Suppress rule for this line / Suppress rule for file quick actions / Show documentation #530

Merged
merged 5 commits into from Oct 19, 2018

Conversation

Projects
None yet
6 participants
@loune
Contributor

loune commented Aug 25, 2018

  • Added Suppress rule for this line / Suppress rule for file quick actions (#245)

eslint

  • Added show documentation quick action

  • Fix possible issue with incorrect command run when 2 rule single fixes are available. This is due to CommandIds.applySingleFix getting overriden inside commands by the last added single fix quick action. The fix is to include the ruleId in the key when adding to commands.

Added Suppress rule for this line / Suppress rule for file quick actions
Fix possible issue with incorrect command run when 2 rule single fixes are available
@msftclas

This comment has been minimized.

msftclas commented Aug 25, 2018

CLA assistant check
All CLA requirements met.

@loune

This comment has been minimized.

Contributor

loune commented Aug 29, 2018

@dbaeumer any chance this could be merged? Thanks

@@ -799,6 +827,14 @@ function validate(document: TextDocument, settings: TextDocumentSettings, publis
if (publishDiagnostics) {
connection.sendDiagnostics({ uri, diagnostics });
}
// cache documentation urls for all rules
ruleDocUrls.clear();

This comment has been minimized.

@dbaeumer

dbaeumer Oct 17, 2018

Member

Is it necessary to cache this on every validate which is actually run very frequently or could we cache this once on only clear the cache if the .eslintrc file changes?

CommandIds.applyAutoFix,
CommandIds.applyDisableLine,
CommandIds.applyDisableFile,
CommandIds.openRuleDoc,

This comment has been minimized.

@dbaeumer

dbaeumer Oct 17, 2018

Member

This is not necessary since the is sent from the server to the client and not the other way.

This comment has been minimized.

@dbaeumer

dbaeumer Oct 17, 2018

Member

I mean CommandIds.openRuleDoc

This comment has been minimized.

@loune

loune Oct 19, 2018

Contributor

When I removed this, the quick action for Show documentation stopped working. I think if we don't specify it here, the command doesn't get sent from quick actions to the server.

This comment has been minimized.

@dbaeumer

dbaeumer Oct 19, 2018

Member

Makes sense. I misunderstood what you wanted to achieve

let workspaceChange = new WorkspaceChange();
workspaceChange.getTextEditChange({uri, version: documentVersion}).add(createTextEdit(editInfo));
commands.set(CommandIds.applySingleFix, workspaceChange);
let lineText = textDocument.getText(Range.create(Position.create(editInfo.line - 1, 0), Position.create(editInfo.line - 1, 255)));

This comment has been minimized.

@dbaeumer

dbaeumer Oct 17, 2018

Member

You can use Integer.MAX_VALUE here instead of 255 to be on the save side. Will have the same effect.

@@ -181,7 +198,8 @@ interface AutoFix {
label: string;

This comment has been minimized.

@dbaeumer

dbaeumer Oct 18, 2018

Member

Since with the change we now store problem in general per file shouldn't we rename this. Something that contains problem in the name

This comment has been minimized.

@dbaeumer

dbaeumer Oct 18, 2018

Member

FixableProblem might be an idea.

loune added some commits Oct 19, 2018

Only clear ruleDocUrls when eslintrc changes
Use MAX_VALUE to get line instead of 255

@loune loune changed the title from Added Suppress rule for this line / Suppress rule for file quick actions to Added Suppress rule for this line / Suppress rule for file quick actions / Show documentation Oct 19, 2018

@dbaeumer

This comment has been minimized.

Member

dbaeumer commented Oct 19, 2018

Very nice work.

@dbaeumer dbaeumer merged commit f040572 into Microsoft:master Oct 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@loune

This comment has been minimized.

Contributor

loune commented Oct 19, 2018

Thanks @dbaeumer !

@loune loune deleted the loune:disable-eslint branch Oct 19, 2018

@dbaeumer

This comment has been minimized.

Member

dbaeumer commented Oct 19, 2018

I did some minor tweaks. One thing I forgot about is that different files can have different .eslintrc files (in different workspace folders) so we need to remember the rules per file. Changed this and pushed.

Will publish a new version of ESLint beginning of next week.

@Friksel

This comment has been minimized.

Friksel commented Nov 5, 2018

Great improvement! Thanks a lot for this!

@TimShilov

This comment has been minimized.

TimShilov commented Nov 6, 2018

Is there an option to hide those additional menu items?
I usually don't suppress rules at all and I don't need the Documentation links
When I open Light Bulb menu I want to only see recommendations and for me those new menu items are just noise.
default

@dbaeumer

This comment has been minimized.

Member

dbaeumer commented Nov 6, 2018

Actually that is a fair request. @loune would you be willing to add such a setting ?

@loune

This comment has been minimized.

Contributor

loune commented Nov 8, 2018

Sure, I can have a look at adding a setting.

@alexiosd

This comment has been minimized.

alexiosd commented Nov 16, 2018

Hello, great addition, but what happened to Fix All of a specific problem?
The Fix all auto-fixable problems could be problematic if you don't know what it will fix.

thank you.

@loune

This comment has been minimized.

Contributor

loune commented Nov 18, 2018

@alexiosd this patch shouldn't change how Fix All of a specific problem works. I'll see what's wrong with that as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment