Conversation
kilifu
commented
Jul 11, 2025
- add overlays option
- caching overlay seams ok
- cleanup
- ad d BinkyLabs.OpenApi.Overlays
There was a problem hiding this comment.
Pull Request Overview
Adds support for processing overlay definitions during client and plugin generation, including command‐line options, server RPC methods, and document application logic.
- Introduced an
overlaysoption/property throughout TypeScript and C# generation interfaces - Extended Kiota CLI commands and handlers to accept and propagate overlays
- Enhanced
OpenApiDocumentDownloadServiceto read, apply, and merge overlay documents, plus added unit tests
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| vscode/packages/npm-package/types.ts | Added overlays to generation configuration types |
| vscode/packages/npm-package/lib/generatePlugin.ts | Propagate overlays into plugin generation API and docs |
| vscode/packages/npm-package/lib/generateClient.ts | Propagate overlays into client generation API and docs |
| tests/Kiota.Builder.Tests/OpenApiDocumentDownloadServiceTests.cs | Added tests for overlay handling in document downloader |
| src/kiota/Rpc/Server.cs | Updated RPC GenerateAsync methods to include overlays parameter |
| src/kiota/Rpc/IServer.cs | Updated interface signatures to include overlays |
| src/kiota/KiotaPluginCommands.cs | Added --overlays option for plugin commands |
| src/kiota/KiotaHost.cs | Added --overlays option for generate command |
| src/kiota/KiotaClientCommands.cs | Added --overlays option for client commands |
| src/kiota/Handlers/Plugin/EditHandler.cs | Wired up OverlaysOption in plugin edit handler |
| src/kiota/Handlers/Plugin/AddHandler.cs | Wired up OverlaysOption in plugin add handler |
| src/kiota/Handlers/KiotaGenerateCommandHandler.cs | Wired up OverlaysOption for generate handler |
| src/kiota/Handlers/Client/EditHandler.cs | Wired up OverlaysOption in client edit handler |
| src/kiota/Handlers/Client/AddHandler.cs | Wired up OverlaysOption in client add handler |
| src/Kiota.Builder/OpenApiDocumentDownloadService.cs | Implemented overlay document loading and application |
| src/Kiota.Builder/Lock/KiotaLock.cs | Added Overlays to lock file serialization |
| src/Kiota.Builder/Kiota.Builder.csproj | Added NuGet package reference for overlays support |
| src/Kiota.Builder/Configuration/GenerationConfiguration.cs | Added Overlays property and included in clone |
Comments suppressed due to low confidence (2)
vscode/packages/npm-package/lib/generateClient.ts:55
- Update the docstring for
overlaysto accurately describe that it is a list of overlay file paths or URLs to apply, e.g., 'List of overlay definitions to apply during client generation.'
* @param {string[]} [clientGenerationOptions.overlays] - Whether the generated client uses overlays.
src/kiota/Rpc/Server.cs:180
- [nitpick] Use the local
configurationvariable consistently instead ofConfiguration.Generationto keep assignments uniform within this method.
configuration.Overlays = new(overlays);
| var fakeLogger = new FakeLogger<OpenApiDocumentDownloadService>(); | ||
|
|
||
|
|
||
| var overlaysPath = Path.GetRandomFileName() + "overlays.yaml"; |
There was a problem hiding this comment.
Ensure the target directory exists before writing the overlay file and use Path.Combine(workingDirectory, "overlays.yaml") for correct filepath; consider calling Directory.CreateDirectory first.
| var overlaysPath = Path.GetRandomFileName() + "overlays.yaml"; | |
| var workingDirectory = Path.Combine(Path.GetTempPath(), "Overlays"); | |
| Directory.CreateDirectory(workingDirectory); | |
| var overlaysPath = Path.Combine(workingDirectory, "overlays.yaml"); |
| if (Directory.Exists(overlaysPath)) | ||
| Directory.Delete(overlaysPath, true); |
There was a problem hiding this comment.
This check uses Directory.Exists on a file path. Switch to File.Exists(overlaysPath) and delete with File.Delete to clean up the overlay file correctly.
| if (Directory.Exists(overlaysPath)) | |
| Directory.Delete(overlaysPath, true); | |
| if (File.Exists(overlaysPath)) | |
| File.Delete(overlaysPath); |
| DisableSSLValidation = DisableSSLValidation, | ||
| ExportPublicApi = ExportPublicApi, | ||
| PluginAuthInformation = PluginAuthInformation, | ||
| Overlays = Overlays |
There was a problem hiding this comment.
The clone method assigns the same Overlays instance rather than copying it. Change to Overlays = Overlays.ToHashSet(StringComparer.OrdinalIgnoreCase) to avoid shared mutable state.
| Overlays = Overlays | |
| Overlays = Overlays?.ToHashSet(StringComparer.OrdinalIgnoreCase) |
| // TODO : handle multiple Overlays | ||
| var overlay = config.Overlays.First(); |
There was a problem hiding this comment.
[nitpick] Either implement support for multiple overlays or remove/update this TODO with a clearer next step to avoid stale comments.
| // TODO : handle multiple Overlays | |
| var overlay = config.Overlays.First(); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| configuration.Deserializers = deserializers.ToHashSet(StringComparer.OrdinalIgnoreCase); | ||
| if (structuredMimeTypes is { Length: > 0 }) | ||
| configuration.StructuredMimeTypes = new(structuredMimeTypes); | ||
| if (overlays is { Length: > 0 }) |
There was a problem hiding this comment.
Why the difference in implementation between the two methods
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>