Repository navigation
Conversation
ab0a18c to
88ac776
Compare
… not yet migrated
88ac776 to
e86791e
Compare
|
do you mind adding a little PR description and ideally an issue reference that explains why we need this and what it does? |
dennisdoomen
added
bug
dennisdoomen
left a comment
dennisdoomen
left a comment
There was a problem hiding this comment.
🔧🤖 No test covers a schema with a NukeBuild definition. Please add one next to TestGetCompletionItemsFromSchema in CompletionUtilityTest.cs.
| { | ||
| if (!TryGetSchema(rootElement, "FalloutBuild", out var baseSchema) && | ||
| // Backward compatibility with NukeBuild projects not yet migrated | ||
| !TryGetSchema(rootElement, "NukeBuild", out baseSchema)) |
There was a problem hiding this comment.
🔧🤖 The negated TryGetSchema chain with the out baseSchema trick is hard to follow. Pick the first of FalloutBuild/NukeBuild found in definitions, then fall back to build.
| if (!definitions.TryGetValue("build", out var buildProperties)) | ||
| { | ||
| // legacy build properties not found ! | ||
| return []; |
There was a problem hiding this comment.
🤔🤖 A missing build definition used to throw KeyNotFoundException and now returns nothing silently. Is that intended? If so, say why; the comment on line 81 doesn't.
| : []); | ||
| } | ||
|
|
||
| private static bool TryGetSchema(JsonElement rootElement, string buildPropertyName, out JsonElement baseSchema) |
There was a problem hiding this comment.
♻️🤖 TryGetSchema can go: definitions is already a dictionary, so use definitions.TryGetValue(name, out var property).
dennisdoomen
changed the title
This branch has not been deployed
3 participants
PR to fix issue #664
Fallout CLI tool auto-completion shall work with NukeBuild projects.
Today, autocompletion searchs only
FalloutBuildproperty inbuild.schema.jsonfile,this PR add support of
NukeBuildproperty