Skip to content

Add python.diagnostics.unresolvedImports setting and IDiagnosticsSettings interface #2049

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Add python.diagnostics.unresolvedImports setting and IDiagnosticsSettings interface #2049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,6 +1266,17 @@
"description": "Path to directory containing the Jedi library (this path will contain the 'Jedi' sub directory).",
"scope": "resource"
},
"python.diagnostics.unresolvedImports": {
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

"type": "string",
"description": "How to display message about unresolved imports.",
"enum": [
"Hint",
"Error",
"Information",
"Warning"
],
"scope": "resource"
},
"python.linting.enabled": {
"type": "boolean",
"default": true,
Expand Down
3 changes: 3 additions & 0 deletions src/client/common/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,9 @@ export interface IMypyCategorySeverity {
readonly error: DiagnosticSeverity;
readonly note: DiagnosticSeverity;
}
export interface IDiagnosticsSettings {
readonly unresolvedImports: DiagnosticSeverity;
}
export interface ILintingSettings {
readonly enabled: boolean;
readonly ignorePatterns: string[];
Expand Down