Refactor our usage of DiagnosticCollector (#1206)

* Refactor our usage of DiagnosticCollector

* fix tests not starting collector

Author: Mpdreamz

src/Elastic.Documentation.Configuration/YamlStreamReader.cs

@@ -16,10 +16,10 @@ public record YamlToplevelKey
 	public required KeyValuePair<YamlNode, YamlNode> Entry { get; init; }
 }
 
-public class YamlStreamReader(IFileInfo source, DiagnosticsCollector collector)
+public class YamlStreamReader(IFileInfo source, IDiagnosticsCollector collector)
 {
 	private IFileInfo Source { get; init; } = source;
-	private DiagnosticsCollector Collector { get; init; } = collector;
+	private IDiagnosticsCollector Collector { get; init; } = collector;
 
 	public IEnumerable<YamlToplevelKey> Read()
 	{
@@ -185,7 +185,7 @@ private void EmitError(string message, Mark? start = null, Mark? end = null, int
 			Column = start.HasValue ? (int)start.Value.Column : null,
 			Length = length
 		};
-		Collector.Channel.Write(d);
+		Collector.Write(d);
 	}
 	public void EmitWarning(string message, Mark? start = null, Mark? end = null, int? length = null)
 	{
@@ -199,6 +199,6 @@ public void EmitWarning(string message, Mark? start = null, Mark? end = null, in
 			Column = start.HasValue ? (int)start.Value.Column : null,
 			Length = length
 		};
-		Collector.Channel.Write(d);
+		Collector.Write(d);
 	}
 }

src/Elastic.Documentation/Diagnostics/DiagnosticsCollector.cs

@@ -8,9 +8,10 @@
 
 namespace Elastic.Documentation.Diagnostics;
 
-public class DiagnosticsCollector(IReadOnlyCollection<IDiagnosticsOutput> outputs) : IHostedService, IAsyncDisposable
+public class DiagnosticsCollector(IReadOnlyCollection<IDiagnosticsOutput> outputs)
+	: IDiagnosticsCollector, IHostedService
 {
-	public DiagnosticsChannel Channel { get; } = new();
+	private DiagnosticsChannel Channel { get; } = new();
 
 	private int _errors;
 	private int _warnings;
@@ -29,7 +30,13 @@ public class DiagnosticsCollector(IReadOnlyCollection<IDiagnosticsOutput> output
 
 	public bool NoHints { get; init; }
 
-	public Task StartAsync(Cancel cancellationToken)
+	public DiagnosticsCollector StartAsync(Cancel ctx)
+	{
+		_ = ((IHostedService)this).StartAsync(ctx);
+		return this;
+	}
+
+	Task IHostedService.StartAsync(Cancel cancellationToken)
 	{
 		if (_started is not null)
 			return _started;
@@ -59,7 +66,6 @@ void Drain()
 			{
 				if (item.Severity == Severity.Hint && NoHints)
 					continue;
-				IncrementSeverityCount(item);
 				HandleItem(item);
 				_ = OffendingFiles.Add(item.File);
 				foreach (var output in outputs)
@@ -82,15 +88,22 @@ protected virtual void HandleItem(Diagnostic diagnostic) { }
 
 	public virtual async Task StopAsync(Cancel cancellationToken)
 	{
+		Channel.TryComplete();
 		if (_started is not null)
 			await _started;
 		await Channel.Reader.Completion;
 	}
 
 	public void EmitCrossLink(string link) => CrossLinks.Add(link);
 
+	public void Write(Diagnostic diagnostic)
+	{
+		IncrementSeverityCount(diagnostic);
+		Channel.Write(diagnostic);
+	}
+
 	private void Emit(Severity severity, string file, string message) =>
-		Channel.Write(new Diagnostic
+		Write(new Diagnostic
 		{
 			Severity = severity,
 			File = file,

src/Elastic.Documentation/Diagnostics/IDiagnosticsCollector.cs

@@ -0,0 +1,40 @@
+// Licensed to Elasticsearch B.V under one or more agreements.
+// Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
+// See the LICENSE file in the project root for more information
+
+using System.Collections.Concurrent;
+using System.IO.Abstractions;
+
+namespace Elastic.Documentation.Diagnostics;
+
+public interface IDiagnosticsCollector : IAsyncDisposable
+{
+	int Warnings { get; }
+	int Errors { get; }
+	int Hints { get; }
+
+	ConcurrentBag<string> CrossLinks { get; }
+	HashSet<string> OffendingFiles { get; }
+	ConcurrentDictionary<string, bool> InUseSubstitutionKeys { get; }
+
+	void EmitError(string file, string message, Exception? e = null);
+	void EmitWarning(string file, string message);
+	void EmitHint(string file, string message);
+	void Write(Diagnostic diagnostic);
+	void CollectUsedSubstitutionKey(ReadOnlySpan<char> key);
+	void EmitCrossLink(string link);
+}
+
+public static class DiagnosticsCollectorExtensions
+{
+	public static void EmitError(this IDiagnosticsCollector collector, IFileInfo file, string message, Exception? e = null) =>
+		collector.EmitError(file.FullName, message, e);
+
+	public static void EmitWarning(this IDiagnosticsCollector collector, IFileInfo file, string message) =>
+		collector.EmitWarning(file.FullName, message);
+
+	public static void EmitHint(this IDiagnosticsCollector collector, IFileInfo file, string message) =>
+		collector.EmitHint(file.FullName, message);
+}
+
+

src/Elastic.Documentation/IDocumentationContext.cs

@@ -9,7 +9,7 @@ namespace Elastic.Documentation;
 
 public interface IDocumentationContext
 {
-	DiagnosticsCollector Collector { get; }
+	IDiagnosticsCollector Collector { get; }
 	IDirectoryInfo DocumentationSourceDirectory { get; }
 	GitCheckoutInformation Git { get; }
 	IFileSystem ReadFileSystem { get; }

src/Elastic.Markdown/BuildContext.cs

@@ -26,7 +26,7 @@ public record BuildContext : IDocumentationContext
 
 	public GitCheckoutInformation Git { get; }
 
-	public DiagnosticsCollector Collector { get; }
+	public IDiagnosticsCollector Collector { get; }
 
 	public bool Force { get; init; }
 
@@ -47,14 +47,11 @@ public string? UrlPathPrefix
 		init => _urlPathPrefix = value;
 	}
 
-	public BuildContext(IFileSystem fileSystem)
-		: this(new DiagnosticsCollector([]), fileSystem, fileSystem, null, null) { }
-
-	public BuildContext(DiagnosticsCollector collector, IFileSystem fileSystem)
+	public BuildContext(IDiagnosticsCollector collector, IFileSystem fileSystem)
 		: this(collector, fileSystem, fileSystem, null, null) { }
 
 	public BuildContext(
-		DiagnosticsCollector collector,
+		IDiagnosticsCollector collector,
 		IFileSystem readFileSystem,
 		IFileSystem writeFileSystem,
 		string? source = null,

src/Elastic.Markdown/Diagnostics/ProcessorDiagnosticExtensions.cs

@@ -30,7 +30,7 @@ public static void EmitError(this InlineProcessor processor, int line, int colum
 			Message = message,
 			Length = length
 		};
-		context.Build.Collector.Channel.Write(d);
+		context.Build.Collector.Write(d);
 	}
 
 
@@ -48,7 +48,7 @@ public static void EmitWarning(this InlineProcessor processor, int line, int col
 			Message = message,
 			Length = length
 		};
-		context.Build.Collector.Channel.Write(d);
+		context.Build.Collector.Write(d);
 	}
 
 	public static void EmitError(this ParserContext context, string message, Exception? e = null)
@@ -61,7 +61,7 @@ public static void EmitError(this ParserContext context, string message, Excepti
 			File = context.MarkdownSourcePath.FullName,
 			Message = CreateExceptionMessage(message, e),
 		};
-		context.Build.Collector.Channel.Write(d);
+		context.Build.Collector.Write(d);
 	}
 
 	public static void EmitWarning(this ParserContext context, int line, int column, int length, string message)
@@ -77,7 +77,7 @@ public static void EmitWarning(this ParserContext context, int line, int column,
 			Message = message,
 			Length = length
 		};
-		context.Build.Collector.Channel.Write(d);
+		context.Build.Collector.Write(d);
 	}
 
 	public static void EmitError(this IBlockExtension block, string message, Exception? e = null) => EmitDiagnostic(block, Severity.Error, message, e);
@@ -100,7 +100,7 @@ private static void EmitDiagnostic(IBlockExtension block, Severity severity, str
 			Length = block.OpeningLength + 5,
 			Message = CreateExceptionMessage(message, e),
 		};
-		block.Build.Collector.Channel.Write(d);
+		block.Build.Collector.Write(d);
 	}
 
 
@@ -121,7 +121,7 @@ private static void LinkDiagnostic(InlineProcessor processor, Severity severity,
 			Message = CreateExceptionMessage(message, e),
 			Length = Math.Max(length, 1)
 		};
-		context.Build.Collector.Channel.Write(d);
+		context.Build.Collector.Write(d);
 	}
 
 	public static void EmitError(this InlineProcessor processor, LinkInline inline, string message) =>

src/Elastic.Markdown/DocumentationGenerator.cs

@@ -117,23 +117,11 @@ public async Task GenerateAll(Cancel ctx)
 		await GenerateLinkReference(ctx);
 	}
 
-	public async Task StopDiagnosticCollection(Cancel ctx)
-	{
-		_logger.LogInformation($"Completing diagnostics channel");
-		Context.Collector.Channel.TryComplete();
-
-		_logger.LogInformation($"Stopping diagnostics collector");
-		await Context.Collector.StopAsync(ctx);
-
-		_logger.LogInformation($"Completed diagnostics channel");
-	}
-
 	private async Task ProcessDocumentationFiles(HashSet<string> offendingFiles, DateTimeOffset outputSeenChanges, Cancel ctx)
 	{
 		var processedFileCount = 0;
 		var exceptionCount = 0;
 		var totalFileCount = DocumentationSet.Files.Count;
-		_ = Context.Collector.StartAsync(ctx);
 		await Parallel.ForEachAsync(DocumentationSet.Files, ctx, async (file, token) =>
 		{
 			var processedFiles = Interlocked.Increment(ref processedFileCount);

src/Elastic.Markdown/Helpers/Interpolation.cs

@@ -31,7 +31,7 @@ ParserContext context
 	public static bool ReplaceSubstitutions(
 		this ReadOnlySpan<char> span,
 		IReadOnlyDictionary<string, string>? properties,
-		DiagnosticsCollector? collector,
+		IDiagnosticsCollector? collector,
 		[NotNullWhen(true)] out string? replacement
 	)
 	{
@@ -43,7 +43,7 @@ public static bool ReplaceSubstitutions(
 	private static bool ReplaceSubstitutions(
 		this ReadOnlySpan<char> span,
 		IReadOnlyDictionary<string, string>[] properties,
-		DiagnosticsCollector? collector,
+		IDiagnosticsCollector? collector,
 		[NotNullWhen(true)] out string? replacement
 	)
 	{

src/Elastic.Markdown/IO/MarkdownFile.cs

@@ -66,7 +66,7 @@ DocumentationSet set
 
 	public string Id { get; } = Guid.NewGuid().ToString("N")[..8];
 
-	private DiagnosticsCollector Collector { get; }
+	private IDiagnosticsCollector Collector { get; }
 
 	public bool Hidden { get; internal set; }
 	public string? UrlPathPrefix { get; }

src/Elastic.Markdown/Links/InboundLinks/LinkIndexLinkChecker.cs

@@ -17,22 +17,22 @@ public class LinkIndexLinkChecker(ILoggerFactory logger)
 
 	private sealed record RepositoryFilter
 	{
-		public string? LinksTo { get; set; }
-		public string? LinksFrom { get; set; }
+		public string? LinksTo { get; init; }
+		public string? LinksFrom { get; init; }
 
 		public static RepositoryFilter None => new();
 	}
 
-	public async Task<int> CheckAll(DiagnosticsCollector collector, Cancel ctx)
+	public async Task CheckAll(IDiagnosticsCollector collector, Cancel ctx)
 	{
 		var fetcher = new LinksIndexCrossLinkFetcher(logger);
 		var resolver = new CrossLinkResolver(fetcher);
 		var crossLinks = await resolver.FetchLinks(ctx);
 
-		return await ValidateCrossLinks(collector, crossLinks, resolver, RepositoryFilter.None, ctx);
+		ValidateCrossLinks(collector, crossLinks, resolver, RepositoryFilter.None);
 	}
 
-	public async Task<int> CheckRepository(DiagnosticsCollector collector, string? toRepository, string? fromRepository, Cancel ctx)
+	public async Task CheckRepository(IDiagnosticsCollector collector, string? toRepository, string? fromRepository, Cancel ctx)
 	{
 		var fetcher = new LinksIndexCrossLinkFetcher(logger);
 		var resolver = new CrossLinkResolver(fetcher);
@@ -43,10 +43,10 @@ public async Task<int> CheckRepository(DiagnosticsCollector collector, string? t
 			LinksFrom = fromRepository
 		};
 
-		return await ValidateCrossLinks(collector, crossLinks, resolver, filter, ctx);
+		ValidateCrossLinks(collector, crossLinks, resolver, filter);
 	}
 
-	public async Task<int> CheckWithLocalLinksJson(DiagnosticsCollector collector, string repository, string localLinksJson, Cancel ctx)
+	public async Task CheckWithLocalLinksJson(IDiagnosticsCollector collector, string repository, string localLinksJson, Cancel ctx)
 	{
 		var fetcher = new LinksIndexCrossLinkFetcher(logger);
 		var resolver = new CrossLinkResolver(fetcher);
@@ -80,17 +80,16 @@ public async Task<int> CheckWithLocalLinksJson(DiagnosticsCollector collector, s
 			LinksTo = repository
 		};
 
-		return await ValidateCrossLinks(collector, crossLinks, resolver, filter, ctx);
+		ValidateCrossLinks(collector, crossLinks, resolver, filter);
 	}
 
-	private async Task<int> ValidateCrossLinks(
-		DiagnosticsCollector collector,
+	private void ValidateCrossLinks(
+		IDiagnosticsCollector collector,
 		FetchedCrossLinks crossLinks,
 		CrossLinkResolver resolver,
-		RepositoryFilter filter,
-		Cancel ctx)
+		RepositoryFilter filter
+	)
 	{
-		_ = collector.StartAsync(ctx);
 		foreach (var (repository, linkReference) in crossLinks.LinkReferences)
 		{
 			if (!string.IsNullOrEmpty(filter.LinksTo))
@@ -130,11 +129,6 @@ private async Task<int> ValidateCrossLinks(
 				}, s => collector.EmitWarning(linksJson, s), uri, out _);
 			}
 		}
-
-		collector.Channel.TryComplete();
-		await collector.StopAsync(ctx);
 		// non-strict for now
-		return collector.Errors;
-		// return collector.Errors + collector.Warnings;
 	}
 }