Add python.diagnostics.unresolvedImports setting and IDiagnosticsSettings interface

[AlexanderSher]

No description provided.

[codecov]

Codecov Report

Merging #2049 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #2049      +/-   ##
=========================================
- Coverage   75.04%     75%   -0.04%     
=========================================
  Files         307     307              
  Lines       14060   14060              
  Branches     2493    2493              
=========================================
- Hits        10551   10546       -5     
- Misses       3373    3376       +3     
- Partials      136     138       +2

Impacted Files	Coverage Δ
src/client/common/types.ts	100% <ø> (ø)	⬆️
...rc/client/debugger/PythonProcessCallbackHandler.ts	52.96% <0%> (-1.32%)	⬇️
src/client/linters/lintingEngine.ts	90.26% <0%> (-0.89%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7334be7...70f1c9d. Read the comment docs.

[DonJayamanne]

Please change to python.linting.unresolveImportsSeverity to line up with what we have today. Other linters too have unresolved imports (at least Pylint).

@brettcannon @MikhailArkhipov
Should we start coming up with our own error codes?
Why?

• This allows us/users to categorize these messages the same way they already do with pylint, pep8, etc using the settings python.linting.pylintCategorySeverity, python.linting.pep8CategorySeverity, python.linting.flake8CategorySeverity, python.linting.mypyCategorySeverity.
• What if the user wanted to disable an unresolved import in one file, or a specific line? E.g. project wide setting is to treat unresolve imports as Errors, however there's an exception to this. This would gives user the ability to ignore specific messages. Yes, this would be an enhancement, and a perfectly valid one. Event .NET does this today (Code Analysis, no reason we shouldn't, seems wrong not to) - hence enhancement)

I.e. the end goal is to be consistent with what we have today and what other linters to today.

[AlexanderSher]

Other linters too have unresolved imports

That's a bit different type of settings. python.linting.pylintCategorySeverity or python.linting.mypyCategorySeverity specify categories to all types of warnings. In our case, it is more detailed setting. Should it be something like python.linting.mpaeCategorySeverity.unresolveImports?

Disabling for a specific line can be done with comments, but I don't know if it is a common practice.

[DonJayamanne]

but I don't know if it is a common practice.

Very common in Python, TypeScript/JavaScript.

That's a bit different type of settings.python.linting.pylintCategorySeverity

The suggestion is to introduce Error Codes, this way we can be consistent with how other linters work and how we let users categorize the messages. I'm certain we'll end up with other messages. Hence the suggestion for error codes.

[AlexanderSher]

Ok, so with error codes, how should we specify in settings that some error should be display as hint instead of warning?

[DonJayamanne]

We provide our own categories and users choose how they are displayed in VSC. E.g. we would categorize unused imports as a warning, and then user would map earrings to errors. That's what we do today with other linters.

[DonJayamanne]

Let's start with something simple, which is what we have today. No one has requested for fine grained control, not yet.

[AlexanderSher]

It is very simple in this case, and additional settings are welcome by users when there is a search mechanism (like in VSC). Error code support is actually more complicated, cause it requires changes in walkers (and maybe in parser) so that we can find those codes.

@brettcannon, @MikhailArkhipov, what are your opinion?

[DonJayamanne]

I think you're missing the point. It's not about what's easy to implement. I don't see the point in coming up with such a fine grained setup, when we don't have anything similar in VSC today. I.e. it won't be used. Let's just KISS.
Any ways, let's discuss next week.

[MikhailArkhipov]

I'd rather separate linting as in pylint etc from the analysis settings. User should know what exactly they are are enabling/disabling. Also, setting for linting would not affect .pylintrc and the other way around, adding to confusion.

Most languages have detailed settings (ex C# CS1234 errors and warnings, C++ has the same). that can be controlled separately. They do no mix with Resharper settings (equivalent of linting in C#) or C++ Lint.

I'd rather see it proper from the start than start changing and morphing.

[MikhailArkhipov]

@DonJayamanne is going to post his version of settings proposal :-).
@qubitron - we may need your opinion here as well.

[DonJayamanne]

@MikhailArkhipov I think we're on the same page except for where the settings should sit (linting vs diagnostics).

@qubitron
Here's my suggestion:

• All diagnostic messages reported by LS need to have an error code, e.g. W1, W2, E1, E2, I1, I2.
  • Where the prefix W, E, I, stand for Warning, Error, Information
• In the case of unused imports, we'd have the code W1.
  • LS treats unused imports as warnings, these are not errors in user code.
  • We could also treat them as informations.
• Now if a user wishes, they can chose to treat all Warnings (reported by LS) as Errors, then all they do is go into the settings and remap Warnings reported by LS into Errors. Note: This is what they already do in the extension today. I.e. we need to be consistent with our approach.
  • "python.linting.languageServer.warning" : "Error"
  • This is very similar to what we have in Big VS as well, Treat all Warnings as Errors (can't remember the exact wording)

I would not want to see settings such as:

• "python.linting.languageServer.1" : "Error"
• "python.linting.languageServer.2" : "Warning"
• "python.linting.languageServer.3" : "Error"
• Where user's map each error code into separate categories. This is unnecessary. As mentioned earlier, user's don't do this today in VSC. Yes, this is possible. But that doesn't mean we should. Its more code to maintain when users will not use it. Lets start with a simple approach. If they want to map each code, then we can consider that as a separate enhancement.

As for custom LS settings (such as code maps, etc). We can add them else where.
We do not need to nest everything under a single place named python.diagnostics.
E.g. if we have code actions for refactoring, we don't have to put them under the above setting. They would go under a something related to refactoring.

[AlexanderSher]

Its more code to maintain when users will not use it

The only additional code the type definition itself. The logic will need to check the setting if it is one for whole app or individual for every Diagnostic message, there is no difference in it.

[DonJayamanne]

message, there is no difference in it.

As mentioned earlier, user's don't do this today in VSC. If users want to map each code, then we can consider that as a separate enhancement, not now.
Also, we're going to have to document this in our online documentation.
I.e. more settings that it will not be used, there's no evidence to back up the need for such a feature.

[DonJayamanne]

@AlexanderSher
Good news, we've agreed to pretty much go down the path of something similar to what you've suggested. Will confirm the requirements with @qubitron @MikhailArkhipov and get back to you. Apologies for the delay.

[MikhailArkhipov]

@AlexanderSher - I am taking over since I am messing with LS settings at the moment anyway (adding typeshed, fixing LS restart conditions, etc)

[MikhailArkhipov]

Closing as it is done in #2065