introduce JsonEmitterBase completion of ( Tweak MetaData Report format for better readability #5929) #6530
Conversation
|
Hi, what is the difference from #5929 and why did you decide to create a new PR? |
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes JSON emission logic by introducing JsonEmitterBase, refactors existing emitters to leverage it, and updates tests and golden files to align with the new JSON structure and cross-platform line-ending normalization.
- Extracted common JSON output routines into
JsonEmitterBaseand adjustedEmitterBase. - Refactored metric, compliance, and metadata emitters to use the new base class and updated their JSON shapes.
- Updated tests: changed
RunGeneratorsignature, addedNormalizeEscapes, switched to verbatim string literals, and refreshed golden JSON files.
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Generators/Shared/JsonEmitterBase.cs | Introduces reusable JSON object/array emission routines. |
| src/Generators/Shared/EmitterBase.cs | Adjusted brace handling and made indent methods support counts. |
| src/Generators/Microsoft.Gen.MetricsReports/MetricDefinitionEmitter.cs | Refactored to inherit from JsonEmitterBase and changed JSON structure. |
| test/Generators/Microsoft.Gen.MetricsReports/Unit/GeneratorTests.cs | Updated RunGenerator signature and added NormalizeEscapes. |
| test/Generators/Microsoft.Gen.MetricsReports/Unit/EmitterTests.cs | Switched expected JSON to a verbatim multi-line string. |
| test/Generators/**/GoldenReports/*.json | Refreshed golden outputs to match the new JSON formatting. |
Comments suppressed due to low confidence (2)
test/Generators/Microsoft.Gen.MetricsReports/Unit/GeneratorTests.cs:147
- Typo in the param description: change 'The coded that the generation will be based-on.' to 'The code that the generation will be based on.'
/// <param name="code">The coded that the generation will be based-on.</param>
test/Generators/Microsoft.Gen.MetricsReports/Unit/EmitterTests.cs:158
- The stray comma on its own line breaks the JSON structure in the expected string literal; remove this extra comma.
,
| protected void OutObject(Action action, bool isProprietyDependent = false) | ||
| { | ||
| NewItem(!isProprietyDependent); |
There was a problem hiding this comment.
The parameter name 'isProprietyDependent' is misspelled; consider renaming it to 'isPropertyDependent' for clarity.
| protected void OutObject(Action action, bool isProprietyDependent = false) | |
| { | |
| NewItem(!isProprietyDependent); | |
| protected void OutObject(Action action, bool isPropertyDependent = false) | |
| { | |
| NewItem(!isPropertyDependent); |
Copilot uses AI. Check for mistakes.
| @@ -34,16 +37,33 @@ public EmitterBase(bool emitPreamble = true) | |||
| } | |||
| } | |||
|
|
|||
| protected void OutOpenBrace() | |||
| protected void OutOpenBrace(bool isRoot = false) // isRoot is used to neglect any extra indentation before the brace, root has no indentation, defaulted to false for backward compatibility. | |||
There was a problem hiding this comment.
[nitpick] The term 'neglect' is ambiguous here; consider rewording to 'omit extra indentation' and adding a proper XML doc comment.
| protected void OutOpenBrace(bool isRoot = false) // isRoot is used to neglect any extra indentation before the brace, root has no indentation, defaulted to false for backward compatibility. | |
| /// <summary> | |
| /// Outputs an opening brace. Optionally omits extra indentation if it is the root. | |
| /// </summary> | |
| /// <param name="isRoot">If true, omits extra indentation before the brace. Defaults to false for backward compatibility.</param> | |
| protected void OutOpenBrace(bool isRoot = false) |
Copilot uses AI. Check for mistakes.
@evgenyfedorov2 Hello there ! , there is no difference between them . the old PR tests kept failing because of the AI.Templates Mocks, |
But aren't they failing in the new PR too? And why can't we just fix the failing tests in the old PR? Just curious |
The Main conflict happened with the files snapshots on Ai.Templates , so i needed to install if you have anything in mind that could help with the old PR please share it with me Thanks |
Microsoft Reviewers: Open in CodeFlow
This pull request is a continuation of the following #5929
As the build was failing to file snapshots for the AI Templates
This pull request only tackle the generated metadata format putting different proprieties on new Lines
Tests were failing due to the differences between escapes in Windows and Linux ,
at the
GeneratorTestsclass in the namespaceMicrosoft.Gen.MetadataExtractor.Unit.Tests;.I introduced new method called
NormalizeEscapes, which will only escape"\r\n"to neglect the gap betweenWindows & Linux, I hope this addition is acceptable.
the test report has been changed to be aligned with the new formatting.
here are the unit tests :
Microsoft Reviewers: Open in CodeFlow