Conversation
There was a problem hiding this comment.
Pull request overview
This PR applies a set of generator and template adjustments focused on (1) reducing unnecessary file writes during generation, (2) improving tag handling for untagged OpenAPI operations, and (3) adding CLI access to third-party notices / cleaning up generated headers.
Changes:
- Skip writing generated outputs / copied artifacts when the on-disk content is already identical.
- Add
--help/--third-party-noticesCLI options and embed third-party notices into the assembly. - Adjust tag collection to include a default tag when operations are tagless; update APL templates (remove generated timestamp header; refine path param validation).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/OpenAPIDyalog/Templates/APLSource/utils.apln.scriban | Removes generated timestamp header; refines isValidPathParam character-vector check. |
| src/OpenAPIDyalog/Templates/APLSource/Client.aplc.scriban | Removes generated timestamp header from generated client class template. |
| src/OpenAPIDyalog/Services/TemplateService.cs | Sets Scriban loop limit; skips writing unchanged rendered output. |
| src/OpenAPIDyalog/Services/ArtifactGeneratorService.cs | Skips rewriting copied HttpCommand/spec when unchanged. |
| src/OpenAPIDyalog/OpenAPIDyalog.csproj | Embeds THIRD_PARTY_NOTICES.txt with a stable logical resource name. |
| src/OpenAPIDyalog/Models/TemplateContext.cs | Includes default tag in GetAllTags() when tagless operations exist. |
| src/OpenAPIDyalog/GeneratorApplication.cs | Adds --help and --third-party-notices options and prints embedded notices. |
| src/OpenAPIDyalog/Constants/GeneratorConstants.cs | Adds resource name constant for third-party notices. |
| mkdocs.yml | Adds version metadata fields for documentation. |
| THIRD_PARTY_NOTICES.txt | Updates licensing wording. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var scriptObject = BuildScriptObject(context); | ||
| var templateContext = new TemplateContext(); | ||
| var templateContext = new TemplateContext { LoopLimit = int.MaxValue }; | ||
| templateContext.PushGlobal(scriptObject); | ||
| return template.Render(templateContext); |
There was a problem hiding this comment.
Setting Scriban TemplateContext.LoopLimit to int.MaxValue disables Scriban’s safeguard against runaway loops. Consider using a high but bounded limit (or making it configurable) so template bugs or unexpectedly large inputs can’t hang the generator indefinitely.
| var scriptObject = BuildScriptObject(context); | ||
| var templateContext = new TemplateContext(); | ||
| var templateContext = new TemplateContext { LoopLimit = int.MaxValue }; | ||
| templateContext.PushGlobal(scriptObject); | ||
| return await template.RenderAsync(templateContext); |
There was a problem hiding this comment.
Same concern as Render(): using LoopLimit = int.MaxValue removes Scriban’s loop protection for async rendering as well. Recommend a bounded/configurable limit to avoid unbounded CPU time if a template loops unexpectedly.
| var srcBytes = await File.ReadAllBytesAsync(sourcePath); | ||
|
|
||
| if (File.Exists(destPath)) | ||
| { | ||
| var destBytes = await File.ReadAllBytesAsync(destPath); | ||
| if (srcBytes.SequenceEqual(destBytes)) | ||
| { |
There was a problem hiding this comment.
CopySpecificationAsync now reads the entire spec file into memory (and potentially the destination too) just to check for equality. For large OpenAPI specs this can be a noticeable memory spike; consider comparing file lengths first and/or doing a streaming/hash-based comparison to avoid loading both files at once.
| var hasTaglessOperations = operations.Any(op => op.Tags == null || op.Tags.Count == 0); | ||
| if (hasTaglessOperations) | ||
| return explicitTags.Append(GeneratorConstants.DefaultTagName).Distinct(); | ||
|
|
There was a problem hiding this comment.
GetAllTags() now uses GeneratorConstants.DefaultTagName for tagless operations, but GetOperationsByTag() in the same class still hard-codes "default". To keep behavior consistent if the default tag name ever changes, use the constant everywhere the default tag is produced/consumed.
| isValidPathParam←{ | ||
| ⍝ True if argument is a character vector or a scalar number | ||
| isChar←(0=10|⎕DR)⍵ | ||
| isChar←(1=≢⍴1∘/⍵)∧(0=10|⎕DR)⍵ | ||
| isScalarNum←(0=≢⍴⍵)∧2|⎕DR ⍵ | ||
| isChar ∨ isScalarNum | ||
| } | ||
| isChar∨isScalarNum | ||
| } |
There was a problem hiding this comment.
The isValidPathParam block is indented differently from the surrounding definitions in this namespace (most are aligned at 4 spaces). Aligning indentation here would keep the generated APL source consistently formatted.
| copyright notices and the licenses under which Dyalog Ltd. received such components | ||
| are set forth below. Dyalog Ltd. licenses these components to you under the terms | ||
| described in this file; Dyalog Ltd. reserves all other rights not expressly granted. | ||
| are set forth below. These components are licensed to you under their respective |
There was a problem hiding this comment.
There’s trailing whitespace at the end of this line (after “respective”). It’s best to remove it to avoid noisy diffs and keep the notices file clean.
| are set forth below. These components are licensed to you under their respective | |
| are set forth below. These components are licensed to you under their respective |
No description provided.