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

[quick fix]: ignore for file #3335

Closed
tjx666 opened this issue May 11, 2021 · 12 comments
Closed

[quick fix]: ignore for file #3335

tjx666 opened this issue May 11, 2021 · 12 comments
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Milestone

Comments

@tjx666
Copy link

tjx666 commented May 11, 2021

dart code:

image

eslint:

image

REF:

How to ignore a lint rule for entire file in Flutter?

@DanTup DanTup added this to the On Deck milestone May 12, 2021
@DanTup DanTup added in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server labels May 12, 2021
@DanTup
Copy link
Member

DanTup commented May 24, 2021

@bwilkerson this "ignore" quick-fix is currently inserted in the client, but I think it probably makes sense to move to the server (and support both line + file together) - I use this a lot in both Dart and TypeScript so suspect others would find it useful.

Do you think it makes sense done outside or LSP and if so, using the same fix API or something separate (eg. should they show up in edit.getFixes)?

@DanTup DanTup modified the milestones: On Deck, v3.24.0 May 24, 2021
@bwilkerson
Copy link

Yes, I think it makes sense to generate it in server. That way we can control which codes it gets generated for (it doesn't make sense, for example, to ignore compiler errors that will prevent code from being run).

If it's going to be in server, then I think it makes sense for them to be returned from edit.getFixes, but we should probably coordinate with the IntelliJ plugin authors to avoid having duplicate fixes. If we/they don't have time for that effort right now, then I'm fine with it going into LSP short term in order to make progress.

@DanTup
Copy link
Member

DanTup commented Jun 9, 2021

@alexander-doroshko @jwren I'm not sure who is best placed to answer this - does IntelliJ have its own way to insert // ignores for lint errors? I'm currently injecting them client-side in VS Code, but would like to move to the server (but don't want dupes showing up). For VS Code I'll probably suppress injecting them if they same things appear in the results from the server, though I'm less sure what to do for IntelliJ (we could just exclude them from edit.getFixes initially, though might want a plan for how they'd be enabled - if appropriate - in future).

@alexander-doroshko
Copy link

alexander-doroshko commented Jun 9, 2021

@bwilkerson @DanTup @jwren Yes, IntelliJ itself adds 'Suppress warning' quick fixes. I'm ok with moving them to server. I hope existing API (edit.getFixes) is sufficient.

There should be some way for the IDE to distinguish 'Suppress...' quick fixes from regular ones. The reason is that IntelliJ's approach is to add 'Suppress...' quick fixes to the secondary menu if a standard quick fix is available. So IDE needs info to tie 'Suppress...' quick fixes to a regular quick fix.
image

I'm happy to coordinate any time to make sure that the Dart plugin stops adding own 'Suppress...' quick fixes starting from the SDK version that adds them itself.

@DanTup
Copy link
Member

DanTup commented Jun 9, 2021

I think edit.getFixes should work, although there might not be many options for indicating it's an ignore-fix. AnalysisErrorFixes contains a List<SourceChange> fixes. We could perhaps prefix the id on the SourceChange with ignore. or similar. Otherwise it might need a tweak to the API. @bwilkerson any thoughts/preferences?

@bwilkerson
Copy link

If using a well known prefix in the id is sufficient for all our clients then that would be the fastest path forward.

@alexander-doroshko
Copy link

Yes, id prefix will work for me.

@DanTup
Copy link
Member

DanTup commented Jun 15, 2021

@bwilkerson @alexander-doroshko I've made a start at this here:

https://dart-review.googlesource.com/c/sdk/+/203760/

Although having the name prefixes doesn't seem simple, as the IDs currently come from the FixKind (and we won't have one of these for every error code). Currently it's generating fixed IDs of dart.fix.ignore.file and dart.fix.ignore.line. Do fixed IDs seem reasonable? (the specific values are easily changed ofc, and I have no preference for the specifics).

@alexander-doroshko if that's ok, how are you planning to disable the built-in ones - can you check for the presence of these, or will you use the version (either SDK or protocol version)? I haven't bumped the protocol version, but could.. the SDK version may be simpler but you may get some overlap with pre-release builds that might/might not have it.

For VS Code, I think I'll check the list of fixes and ensure there are no existing ignore fixes before injecting locally (and sometime after it ships in stable, just remove the code that injects them locally).

Here's an example of the fixes in the original protocol:

{
	"error": {
		"severity": "INFO",
		"type": "HINT",
		"location": {
			"file": "/Users/danny/Desktop/dart_sample/bin/asyncstep.dart",
			"offset": 463,
			"length": 10,
			"startLine": 23,
			"startColumn": 5,
			"endLine": 23,
			"endColumn": 15
		},
		"message": "Dead code.",
		"correction": "Try removing the code, or fixing the code before it so that it can be reached.",
		"code": "dead_code",
		"url": "https://dart.dev/diagnostics/dead_code",
		"hasFix": true
	},
	"fixes": [
		{
			"message": "Remove dead code",
			"edits": [
				{
					"file": "/Users/danny/Desktop/dart_sample/bin/asyncstep.dart",
					"fileStamp": 0,
					"edits": [
						{
							"offset": 459,
							"length": 15,
							"replacement": ""
						}
					]
				}
			],
			"linkedEditGroups": [],
			"id": "dart.fix.remove.deadCode"
		},
		{
			"message": "Ignore 'dead_code' for this line",
			"edits": [
				{
					"file": "/Users/danny/Desktop/dart_sample/bin/asyncstep.dart",
					"fileStamp": 0,
					"edits": [
						{
							"offset": 459,
							"length": 0,
							"replacement": "    // ignore: dead_code\n"
						}
					]
				}
			],
			"linkedEditGroups": [],
			"id": "dart.fix.ignore.line"
		},
		{
			"message": "Ignore 'dead_code' for this file",
			"edits": [
				{
					"file": "/Users/danny/Desktop/dart_sample/bin/asyncstep.dart",
					"fileStamp": 0,
					"edits": [
						{
							"offset": 0,
							"length": 0,
							"replacement": "// ignore_for_file: dead_code\n\n"
						}
					]
				}
			],
			"linkedEditGroups": [],
			"id": "dart.fix.ignore.file"
		}
	]
}

@alexander-doroshko
Copy link

how are you planning to disable the built-in ones - can you check for the presence of these

Yes, this should work for me. Thank you for this work!

@DanTup
Copy link
Member

DanTup commented Jun 15, 2021

@alexander-doroshko great - let me know if/when you're happy for it land in the SDK. I'll sort the VS Code side this week. Thanks!

@alexander-doroshko
Copy link

Any time, it won't break things. I'll update the Dart plugin shortly.

@DanTup
Copy link
Member

DanTup commented Jun 17, 2021

This work has landed in the SDK:

dart-lang/sdk@2e411e5

In a future SDK release, there will be "ignore for line" and "ignore for file" fixes for lints and hints. VS Code's own local versions of those fixes will automatically be hidden when the server provides them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Projects
None yet
Development

No branches or pull requests

4 participants