-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Pylint not enabled by default, can be easily enabled for workspaces that have it available. #2913
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
Conversation
900dcb2
to
282b117
Compare
public isFeatureEnabled(): boolean { | ||
const ws = this.workspaceConfig.getConfiguration('python'); | ||
const jediEnabled = ws!.inspect('jediEnabled'); | ||
return (!jediEnabled || jediEnabled.defaultValue === false); |
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 use IConfigurationService
instead, that's strongly typed.
You don't need to use inspect
and such low level checks.
@inject(IConfigurationService) private readonly configurationServivce
.... in the constructor
Then, use the following code:
public get isFeatureEnabled(){
return this.configurationService.getSettings().jediEnabled;
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.
Unfortunately in this case we need the lower level.
Our new availability check feature is enabled only after python.jediEnabled default value is false.
Our availability check will only prompt when the linter being checked (pylint
for now, maybe others later) has not been configured by the user in any way yet.
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.
Ok, slight misunderstanding here - getting the python.jediEnabled
setting is definitely better off using the IConfigurationService
so we can enable this feature for all people using the Python Language Server, not just when we make it the default. Made the change.
This comment has been minimized.
This comment has been minimized.
ecc3353
to
fac640a
Compare
Fix for microsoft#974 - Pylint is easily enabled for workspaces that have it available
- Revert removal of skip() from test suite. - Fix naming of a few methods - Update method documentation to jsdocs format
- flag changed from `checkAvailable` to `silent` - affects calls to `getActiveLinters` and `isLintingEnabled`
- new class `AvailableLinterActivator` - DI enable the new class - correct some issues with DI usage in the original logic
- add check for 'is feature enabled' and test for it too - feature is enabled when python.jediEnabled=false by default
- Also fixed up tests to make them far simpler to read.
- When determining the jediEnabled state. - Turns this feature on for anyone using the MPLS
- ensure only valid linters can be set (we can't default to pylint anymore) - ensure await is called for isLintingEnabled.
- update test suite setup - include the new linter availability dependancy - correct linterProvider to be async onDocumentSave - remove previous debugging-only code from linterManager!
- Linting cannot block saving
f5cebf5
to
350e143
Compare
Fix for #974
pylint
is easily enabled for workspaces that have it available, but it is no longer set at the global default for the vscode-python extension.package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed)