Chrisdias/linting #42
Conversation
| @@ -1,2 +1,3 @@ | |||
| out | |||
| out/**/*.js | |||
| out/**/*.map | |||
There was a problem hiding this comment.
Can we ignore the yaml files that are in the out directory as well? It seems like the extension doesn't use them.
There was a problem hiding this comment.
actually it does use the .yaml files, they are the linting rules. i had to change gitignore to persist them. happy if you have another way to do this.
| } | ||
| } | ||
|
|
||
| diagnosticCollection.set(document.uri, diagnostics); |
There was a problem hiding this comment.
Should we do anything with the info-level linting messages? It seems like the default rules files include some.
There was a problem hiding this comment.
also forgot to do this :)
| @@ -54,4 +58,16 @@ export function activate(ctx: vscode.ExtensionContext): void { | |||
| ctx.subscriptions.push(vscode.commands.registerCommand('vscode-docker.container.open-shell', openShellContainer)); | |||
| ctx.subscriptions.push(vscode.commands.registerCommand('vscode-docker.compose.up', composeUp)); | |||
| ctx.subscriptions.push(vscode.commands.registerCommand('vscode-docker.compose.down', composeDown)); | |||
|
|
|||
| diagnosticCollection = vscode.languages.createDiagnosticCollection('docker-diagnostics'); | |||
There was a problem hiding this comment.
Did we want to check whether linting was enabled? I noticed you added a config option to turn it on/off, but didn't see if it was checked before scheduling linting validations.
| reference_url: | ||
| - "https://docs.docker.com/reference/builder/" | ||
| - "#from" | ||
| label: "no_tag" |
There was a problem hiding this comment.
Is this rule and the one below it missing their hyphen delimiter? The rules for the RUN directive below each have one, which I had thought was the correct syntax for lists in yaml.
There was a problem hiding this comment.
these came with dockerfile_lint so i don't know. but, we can remove all these per the above comments.
| let diagnostics: vscode.Diagnostic[] = []; | ||
|
|
||
| let linterRuleFile = configOptions.get('linterRuleFile', 'basic_rules.yaml'); | ||
|
|
There was a problem hiding this comment.
It looks like this lib will actually default to using this exact rules file if you specific a null/empty rules file path, so we could choose not to ship any of these sample rules file, and allow either turning on the default rules or specifying a custom one.
There was a problem hiding this comment.
interesting... i would rather remove the rules files and go with the defaults. but, i'll keep the ability to create a new rules file and drop into the root of the workspace. or, i'll go with a full path so you could have 1 on the machine...
There was a problem hiding this comment.
"fixed" by not relying on file...
|
great feedback! i've still got work to do, will make changes and then resubmit. |
|
LGTM! |
adds linting for dockerfiles using dockerfile_lint library