From 1a07fd15d73a91f0efd8feb84f4682ab36e5d80e Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Mon, 13 Jun 2022 17:06:28 -0700 Subject: [PATCH] Clarify markdown validate settings (#151997) Clairify markdown validate settings Fixes #150949 - Rename headerLinks -> fragmentLinks - Add new `fileLink.markdownFragmentsLinks` to validate the headers on fragment links (inherits the default setting value from `fragmentLinks`) --- .../markdown-language-features/package.json | 17 +++- .../package.nls.json | 3 +- .../src/languageFeatures/diagnostics.ts | 40 +++++---- .../src/test/diagnostic.test.ts | 90 +++++++++++++------ 4 files changed, 101 insertions(+), 49 deletions(-) diff --git a/extensions/markdown-language-features/package.json b/extensions/markdown-language-features/package.json index f78e85fa0cb..547983e3a11 100644 --- a/extensions/markdown-language-features/package.json +++ b/extensions/markdown-language-features/package.json @@ -447,10 +447,10 @@ "experimental" ] }, - "markdown.experimental.validate.headerLinks.enabled": { + "markdown.experimental.validate.fragmentLinks.enabled": { "type": "string", "scope": "resource", - "markdownDescription": "%configuration.markdown.experimental.validate.headerLinks.enabled.description%", + "markdownDescription": "%configuration.markdown.experimental.validate.fragmentLinks.enabled.description%", "default": "warning", "enum": [ "ignore", @@ -475,6 +475,19 @@ "experimental" ] }, + "markdown.experimental.validate.fileLinks.markdownFragmentLinks": { + "type": "string", + "scope": "resource", + "markdownDescription": "%configuration.markdown.experimental.validate.fileLinks.markdownFragmentLinks.description%", + "enum": [ + "ignore", + "warning", + "error" + ], + "tags": [ + "experimental" + ] + }, "markdown.experimental.validate.ignoreLinks": { "type": "array", "scope": "resource", diff --git a/extensions/markdown-language-features/package.nls.json b/extensions/markdown-language-features/package.nls.json index c7c4c6e5c21..808d4e77cb9 100644 --- a/extensions/markdown-language-features/package.nls.json +++ b/extensions/markdown-language-features/package.nls.json @@ -32,8 +32,9 @@ "configuration.markdown.editor.pasteLinks.enabled": "Enable/disable pasting files into a Markdown editor inserts Markdown links.", "configuration.markdown.experimental.validate.enabled.description": "Enable/disable all error reporting in Markdown files.", "configuration.markdown.experimental.validate.referenceLinks.enabled.description": "Validate reference links in Markdown files, e.g. `[link][ref]`. Requires enabling `#markdown.experimental.validate.enabled#`.", - "configuration.markdown.experimental.validate.headerLinks.enabled.description": "Validate links to headers in Markdown files, e.g. `[link](#header)`. Requires enabling `#markdown.experimental.validate.enabled#`.", + "configuration.markdown.experimental.validate.fragmentLinks.enabled.description": "Validate fragment links to headers in the current Markdown file, e.g. `[link](#header)`. Requires enabling `#markdown.experimental.validate.enabled#`.", "configuration.markdown.experimental.validate.fileLinks.enabled.description": "Validate links to other files in Markdown files, e.g. `[link](/path/to/file.md)`. This checks that the target files exists. Requires enabling `#markdown.experimental.validate.enabled#`.", + "configuration.markdown.experimental.validate.fileLinks.markdownFragmentLinks.description": "Validate the fragment part of links to headers in other files in Markdown files, e.g. `[link](/path/to/file.md#header)`. Inherits the setting value from `#markdown.experimental.validate.fragmentLinks.enabled#` by default.", "configuration.markdown.experimental.validate.ignoreLinks.description": "Configure links that should not be validated. For example `/about` would not validate the link `[about](/about)`, while the glob `/assets/**/*.svg` would let you skip validation for any link to `.svg` files under the `assets` directory.", "workspaceTrust": "Required for loading styles configured in the workspace." } diff --git a/extensions/markdown-language-features/src/languageFeatures/diagnostics.ts b/extensions/markdown-language-features/src/languageFeatures/diagnostics.ts index ed9b174ffb3..b94ca0f5227 100644 --- a/extensions/markdown-language-features/src/languageFeatures/diagnostics.ts +++ b/extensions/markdown-language-features/src/languageFeatures/diagnostics.ts @@ -37,17 +37,19 @@ export enum DiagnosticLevel { export interface DiagnosticOptions { readonly enabled: boolean; - readonly validateReferences: DiagnosticLevel; - readonly validateOwnHeaders: DiagnosticLevel; - readonly validateFilePaths: DiagnosticLevel; + readonly validateReferences: DiagnosticLevel | undefined; + readonly validateFragmentLinks: DiagnosticLevel | undefined; + readonly validateFileLinks: DiagnosticLevel | undefined; + readonly validateMarkdownFileLinkFragments: DiagnosticLevel | undefined; readonly ignoreLinks: readonly string[]; } -function toSeverity(level: DiagnosticLevel): vscode.DiagnosticSeverity | undefined { +function toSeverity(level: DiagnosticLevel | undefined): vscode.DiagnosticSeverity | undefined { switch (level) { case DiagnosticLevel.error: return vscode.DiagnosticSeverity.Error; case DiagnosticLevel.warning: return vscode.DiagnosticSeverity.Warning; case DiagnosticLevel.ignore: return undefined; + case undefined: return undefined; } } @@ -63,8 +65,9 @@ class VSCodeDiagnosticConfiguration extends Disposable implements DiagnosticConf if ( e.affectsConfiguration('markdown.experimental.validate.enabled') || e.affectsConfiguration('markdown.experimental.validate.referenceLinks.enabled') - || e.affectsConfiguration('markdown.experimental.validate.headerLinks.enabled') + || e.affectsConfiguration('markdown.experimental.validate.fragmentLinks.enabled') || e.affectsConfiguration('markdown.experimental.validate.fileLinks.enabled') + || e.affectsConfiguration('markdown.experimental.validate.fileLinks.markdownFragmentLinks') || e.affectsConfiguration('markdown.experimental.validate.ignoreLinks') ) { this._onDidChange.fire(); @@ -74,11 +77,13 @@ class VSCodeDiagnosticConfiguration extends Disposable implements DiagnosticConf public getOptions(resource: vscode.Uri): DiagnosticOptions { const config = vscode.workspace.getConfiguration('markdown', resource); + const validateFragmentLinks = config.get('experimental.validate.fragmentLinks.enabled'); return { enabled: config.get('experimental.validate.enabled', false), - validateReferences: config.get('experimental.validate.referenceLinks.enabled', DiagnosticLevel.ignore), - validateOwnHeaders: config.get('experimental.validate.headerLinks.enabled', DiagnosticLevel.ignore), - validateFilePaths: config.get('experimental.validate.fileLinks.enabled', DiagnosticLevel.ignore), + validateReferences: config.get('experimental.validate.referenceLinks.enabled'), + validateFragmentLinks, + validateFileLinks: config.get('experimental.validate.fileLinks.enabled'), + validateMarkdownFileLinkFragments: config.get('markdown.experimental.validate.fileLinks.markdownFragmentLinks', validateFragmentLinks), ignoreLinks: config.get('experimental.validate.ignoreLinks', []), }; } @@ -294,7 +299,7 @@ export class DiagnosticManager extends Disposable { if (doc) { this.inFlightDiagnostics.trigger(doc.uri, async (token) => { const state = await this.recomputeDiagnosticState(doc, token); - this.linkWatcher.updateLinksForDocument(doc.uri, state.config.enabled && state.config.validateFilePaths ? state.links : []); + this.linkWatcher.updateLinksForDocument(doc.uri, state.config.enabled && state.config.validateFileLinks ? state.links : []); this.collection.set(doc.uri, state.diagnostics); }); } @@ -395,13 +400,13 @@ export class DiagnosticComputer { diagnostics: (await Promise.all([ this.validateFileLinks(doc, options, links, token), Array.from(this.validateReferenceLinks(options, links)), - this.validateOwnHeaderLinks(doc, options, links, token), + this.validateFragmentLinks(doc, options, links, token), ])).flat() }; } - private async validateOwnHeaderLinks(doc: SkinnyTextDocument, options: DiagnosticOptions, links: readonly MdLink[], token: vscode.CancellationToken): Promise { - const severity = toSeverity(options.validateOwnHeaders); + private async validateFragmentLinks(doc: SkinnyTextDocument, options: DiagnosticOptions, links: readonly MdLink[], token: vscode.CancellationToken): Promise { + const severity = toSeverity(options.validateFragmentLinks); if (typeof severity === 'undefined') { return []; } @@ -449,10 +454,11 @@ export class DiagnosticComputer { } private async validateFileLinks(doc: SkinnyTextDocument, options: DiagnosticOptions, links: readonly MdLink[], token: vscode.CancellationToken): Promise { - const severity = toSeverity(options.validateFilePaths); - if (typeof severity === 'undefined') { + const pathErrorSeverity = toSeverity(options.validateFileLinks); + if (typeof pathErrorSeverity === 'undefined') { return []; } + const fragmentErrorSeverity = toSeverity(typeof options.validateMarkdownFileLinkFragments === 'undefined' ? options.validateFragmentLinks : options.validateMarkdownFileLinkFragments); const linkSet = new FileLinkMap(links); if (linkSet.size === 0) { @@ -479,10 +485,10 @@ export class DiagnosticComputer { const msg = localize('invalidPathLink', 'File does not exist at path: {0}', path.fsPath); for (const link of links) { if (!this.isIgnoredLink(options, link.source.pathText)) { - diagnostics.push(new LinkDoesNotExistDiagnostic(link.source.hrefRange, msg, severity, link.source.pathText)); + diagnostics.push(new LinkDoesNotExistDiagnostic(link.source.hrefRange, msg, pathErrorSeverity, link.source.pathText)); } } - } else if (hrefDoc) { + } else if (hrefDoc && typeof fragmentErrorSeverity !== 'undefined') { // Validate each of the links to headers in the file const fragmentLinks = links.filter(x => x.fragment); if (fragmentLinks.length) { @@ -490,7 +496,7 @@ export class DiagnosticComputer { for (const link of fragmentLinks) { if (!toc.lookup(link.fragment) && !this.isIgnoredLink(options, link.source.pathText) && !this.isIgnoredLink(options, link.source.text)) { const msg = localize('invalidLinkToHeaderInOtherFile', 'Header does not exist in file: {0}', link.fragment); - diagnostics.push(new LinkDoesNotExistDiagnostic(link.source.hrefRange, msg, severity, link.source.text)); + diagnostics.push(new LinkDoesNotExistDiagnostic(link.source.hrefRange, msg, fragmentErrorSeverity, link.source.text)); } } } diff --git a/extensions/markdown-language-features/src/test/diagnostic.test.ts b/extensions/markdown-language-features/src/test/diagnostic.test.ts index d84a69dd20c..038ad1c55f5 100644 --- a/extensions/markdown-language-features/src/test/diagnostic.test.ts +++ b/extensions/markdown-language-features/src/test/diagnostic.test.ts @@ -23,15 +23,16 @@ async function getComputedDiagnostics(doc: InMemoryDocument, workspaceContents: return ( await computer.getDiagnostics(doc, { enabled: true, - validateFilePaths: DiagnosticLevel.warning, - validateOwnHeaders: DiagnosticLevel.warning, + validateFileLinks: DiagnosticLevel.warning, + validateFragmentLinks: DiagnosticLevel.warning, + validateMarkdownFileLinkFragments: DiagnosticLevel.warning, validateReferences: DiagnosticLevel.warning, ignoreLinks: [], }, noopToken) ).diagnostics; } -function createDiagnosticsManager(workspaceContents: MdWorkspaceContents, configuration = new MemoryDiagnosticConfiguration()) { +function createDiagnosticsManager(workspaceContents: MdWorkspaceContents, configuration = new MemoryDiagnosticConfiguration({})) { const engine = createNewMarkdownEngine(); const linkProvider = new MdLinkProvider(engine); return new DiagnosticManager(new DiagnosticComputer(engine, workspaceContents, linkProvider), configuration); @@ -45,32 +46,28 @@ function assertDiagnosticsEqual(actual: readonly vscode.Diagnostic[], expectedRa } } +const defaultDiagnosticsOptions = Object.freeze({ + enabled: true, + validateFileLinks: DiagnosticLevel.warning, + validateMarkdownFileLinkFragments: undefined, + validateFragmentLinks: DiagnosticLevel.warning, + validateReferences: DiagnosticLevel.warning, + ignoreLinks: [], +}); + class MemoryDiagnosticConfiguration implements DiagnosticConfiguration { private readonly _onDidChange = new vscode.EventEmitter(); public readonly onDidChange = this._onDidChange.event; constructor( - private readonly enabled: boolean = true, - private readonly ignoreLinks: string[] = [], + private readonly _options: Partial, ) { } getOptions(_resource: vscode.Uri): DiagnosticOptions { - if (!this.enabled) { - return { - enabled: false, - validateFilePaths: DiagnosticLevel.ignore, - validateOwnHeaders: DiagnosticLevel.ignore, - validateReferences: DiagnosticLevel.ignore, - ignoreLinks: this.ignoreLinks, - }; - } return { - enabled: true, - validateFilePaths: DiagnosticLevel.warning, - validateOwnHeaders: DiagnosticLevel.warning, - validateReferences: DiagnosticLevel.warning, - ignoreLinks: this.ignoreLinks, + ...defaultDiagnosticsOptions, + ...this._options, }; } } @@ -172,7 +169,7 @@ suite('markdown: Diagnostics', () => { `[text][no-such-ref]`, )); - const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration(false)); + const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration({ enabled: false })); const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); assert.deepStrictEqual(diagnostics.length, 0); }); @@ -203,17 +200,52 @@ suite('markdown: Diagnostics', () => { `[text]: /no-such-file`, )); - const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration(true, ['/no-such-file'])); + const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration({ ignoreLinks: ['/no-such-file'] })); const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); assert.deepStrictEqual(diagnostics.length, 0); }); + test('Should be able to disable fragment validation for external files', async () => { + const doc1 = new InMemoryDocument(workspacePath('doc1.md'), joinLines( + `![i](/doc2.md#no-such)`, + )); + const doc2 = new InMemoryDocument(workspacePath('doc2.md'), joinLines('')); + + const contents = new InMemoryWorkspaceMarkdownDocuments([doc1, doc2]); + + const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration({ validateMarkdownFileLinkFragments: DiagnosticLevel.ignore })); + const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); + assert.deepStrictEqual(diagnostics.length, 0); + }); + + test('Disabling own fragment validation should also disable path fragment validation by default', async () => { + const doc1 = new InMemoryDocument(workspacePath('doc1.md'), joinLines( + `[b](#no-head)`, + `![i](/doc2.md#no-such)`, + )); + const doc2 = new InMemoryDocument(workspacePath('doc2.md'), joinLines('')); + + const contents = new InMemoryWorkspaceMarkdownDocuments([doc1, doc2]); + + { + const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration({ validateFragmentLinks: DiagnosticLevel.ignore })); + const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); + assert.deepStrictEqual(diagnostics.length, 0); + } + { + // But we should be able to override the default + const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration({ validateFragmentLinks: DiagnosticLevel.ignore, validateMarkdownFileLinkFragments: DiagnosticLevel.warning })); + const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); + assert.deepStrictEqual(diagnostics.length, 1); + } + }); + test('ignoreLinks should allow skipping link to non-existent file', async () => { const doc1 = new InMemoryDocument(workspacePath('doc1.md'), joinLines( `[text](/no-such-file#header)`, )); - const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration(true, ['/no-such-file'])); + const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration({ ignoreLinks: ['/no-such-file'] })); const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); assert.deepStrictEqual(diagnostics.length, 0); }); @@ -223,7 +255,7 @@ suite('markdown: Diagnostics', () => { `[text](/no-such-file#header)`, )); - const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration(true, ['/no-such-file'])); + const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration({ ignoreLinks: ['/no-such-file'] })); const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); assert.deepStrictEqual(diagnostics.length, 0); }); @@ -235,7 +267,7 @@ suite('markdown: Diagnostics', () => { `![i](/images/sub/sub2/ccc.png)`, )); - const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration(true, ['/images/**/*.png'])); + const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration({ ignoreLinks: ['/images/**/*.png'] })); const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); assert.deepStrictEqual(diagnostics.length, 0); }); @@ -245,7 +277,7 @@ suite('markdown: Diagnostics', () => { `![i](#no-such)`, )); - const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration(true, ['#no-such'])); + const manager = createDiagnosticsManager(new InMemoryWorkspaceMarkdownDocuments([doc1]), new MemoryDiagnosticConfiguration({ ignoreLinks: ['#no-such'] })); const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); assert.deepStrictEqual(diagnostics.length, 0); }); @@ -258,12 +290,12 @@ suite('markdown: Diagnostics', () => { const contents = new InMemoryWorkspaceMarkdownDocuments([doc1, doc2]); { - const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration(true, ['/doc2.md#no-such'])); + const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration({ ignoreLinks: ['/doc2.md#no-such'] })); const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); assert.deepStrictEqual(diagnostics.length, 0); } { - const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration(true, ['/doc2.md#*'])); + const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration({ ignoreLinks: ['/doc2.md#*'] })); const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); assert.deepStrictEqual(diagnostics.length, 0); } @@ -276,7 +308,7 @@ suite('markdown: Diagnostics', () => { const doc2 = new InMemoryDocument(workspacePath('doc2.md'), joinLines('')); const contents = new InMemoryWorkspaceMarkdownDocuments([doc1, doc2]); - const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration(true, ['/doc2.md'])); + const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration({ ignoreLinks: ['/doc2.md'] })); const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); assert.deepStrictEqual(diagnostics.length, 0); }); @@ -289,7 +321,7 @@ suite('markdown: Diagnostics', () => { )); const contents = new InMemoryWorkspaceMarkdownDocuments([doc1]); - const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration(true, ['/doc2.md'])); + const manager = createDiagnosticsManager(contents, new MemoryDiagnosticConfiguration({ ignoreLinks: ['/doc2.md'] })); const { diagnostics } = await manager.recomputeDiagnosticState(doc1, noopToken); assert.deepStrictEqual(diagnostics.length, 0); });