Tweak MetaData Report format for better readability #5929
Conversation
…ws and linux in tests
…/extensions into audit-report-format
|
the checks failed because of something related to hybrid caching |
src/Generators/Microsoft.Gen.MetadataExtractor/MetadataReportsGenerator.cs
Outdated
Show resolved
Hide resolved
|
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=956346&view=codecoverage-tab |
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=957466&view=codecoverage-tab |
Could you please also bump this threshold? |
src/Generators/Microsoft.Gen.MetadataExtractor/MetadataReportsGenerator.cs
Outdated
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.MetadataExtractor/MetadataReportsGenerator.cs
Outdated
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.MetricsReports/MetricsReportsGenerator.cs
Outdated
Show resolved
Hide resolved
test/Generators/Microsoft.Gen.MetadataExtractor/Unit/GeneratorTests.cs
Outdated
Show resolved
Hide resolved
test/Generators/Microsoft.Gen.MetadataExtractor/Unit/GeneratorTests.cs
Outdated
Show resolved
Hide resolved
RussKie
added
waiting-author-feedback 📭
|
@RussKie Thank you for your valuable feedback, I will fix them ASAP , regarding the changing the order of parameters and naming ,I tried to make them consistent with compliance. now all of the are consistent , I could change the three of them if you'd like. I'll work on the other notes in the meantime |
dotnet-policy-service
bot
removed
the
waiting-author-feedback 📭
-fix spaces -fix documentation
…/extensions into audit-report-format
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=957592&view=codecoverage-tab |
…/extensions into audit-report-format
| "File": "src-0.cs", | ||
| "Line": "18", | ||
| "Classifications": [ | ||
| "Types": [ |
There was a problem hiding this comment.
This indentation doesn't look correct
There was a problem hiding this comment.
its has been indented , it looks like this currently
level2 props is indented a bit to make it standout , if you have any thoughts ill reflect it
{
"Name":"test.dll",
"ComplianceReport":
{
"Types": [
{
"Name": "Test.Basic",
"Members": [
{
"Name": "F0",
"Type": "int",
"File": "src-0.cs",
"Line": "18",
"Classifications": [
{
"Name": "C1"
},
{
"Name": "C2",
"Notes": "Note 1"
}
]
},
{
"Name": "F1",
"Type": "int",
"File": "src-0.cs",
"Line": "21",
"Classifications": [
{
"Name": "C1"
},
{
"Name": "C2"
}
]
},
{
"Name": "P0",
"Type": "int",
"File": "src-0.cs",
"Line": "11",
"Classifications": [
{
"Name": "C1"
},
{
"Name": "C3",
"Notes": "Note 2"
},
{
"Name": "C4"
}
]
},
{
"Name": "P1",
"Type": "int",
"File": "src-0.cs",
"Line": "27",
"Classifications": [
{
"Name": "C1"
},
{
"Name": "C3"
}
]
}
],
"Logging Methods": [
{
"Name": "LogHello",
"Parameters": [
{
"Name": "user",
"Type": "string",
"File": "src-0.cs",
"Line": "30",
"Classifications": [
{
"Name": "C2",
"Notes": "Note 3"
}
]
},
{
"Name": "port",
"Type": "int",
"File": "src-0.cs",
"Line": "30"
}
]
},
{
"Name": "LogWorld",
"Parameters": [
{
"Name": "user",
"Type": "string",
"File": "src-0.cs",
"Line": "33",
"Classifications": [
{
"Name": "C2"
}
]
},
{
"Name": "port",
"Type": "int",
"File": "src-0.cs",
"Line": "33"
}
]
}
]
}
]
},
"MetricReport": []
}
There was a problem hiding this comment.
The first level properties aren't indented. The 3rd level onwards "{" are indented by 4 spaces, and the 2nd level is indented by 16 (?) spaces. This is what caught my eye.
I would kind of expect something like
{
"Name":"test.dll",
"ComplianceReport":
{
"Types": [
{
"Name": "Test.Basic",(Although my personal preference would be 2 spaces :))
There was a problem hiding this comment.
I have fixed it , it's actually a lot better now , thank you
please for you verification - this is taken from Basic.json
{
"Name":"test.dll",
"ComplianceReport":
{
"Types": [
{
"Name": "Test.Basic",
|
Looks better 👍 |
src/Generators/Microsoft.Gen.MetadataExtractor/MetadataEmitter.cs
Outdated
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.MetadataExtractor/MetadataEmitter.cs
Outdated
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.MetadataExtractor/MetadataEmitter.cs
Outdated
Show resolved
Hide resolved
src/Generators/Microsoft.Gen.MetadataExtractor/MetadataEmitter.cs
Outdated
Show resolved
Hide resolved
...rators/Microsoft.Gen.MetadataExtractor/GoldenReports/MeterAttributedWithXmlDescriptions.json
Outdated
Show resolved
Hide resolved
test/Generators/Microsoft.Gen.MetadataExtractor/GoldenReports/Basic.json
Outdated
Show resolved
Hide resolved
| { | ||
| "TestClasses": | ||
| [ |
There was a problem hiding this comment.
Indentations in this file look doubled
There was a problem hiding this comment.
sorry , I cant follow on this one , can u explain more ?
all other notes are fixed
There was a problem hiding this comment.
Also, could you please help me understand why we're changing from 2 spaces to 4 spaces?
There was a problem hiding this comment.
at the EmitterBase its set to 4
this is the reason of indenting 4 instead of two
however , I made sure its consistence what ever its set in the EmitterBase
There was a problem hiding this comment.
Emitter base also affects Logging generation, which is C# code, and thus 4 spaces is ideal. I wouldn't change this value.
There was a problem hiding this comment.
a suggestion would be to make it virtual, and override it whenever its needed.
However , this would require us to revisit all three reports to tune the unit testing files etc....
There was a problem hiding this comment.
Suggestion I made below was to create a EmitterJsonBase which inherits EmitterBase. Then it can adjust this if we want to have the 2 spaces per indent level. It also can include helpers related to JSON output (OutArray, OutObject, OutNameValue)
...rators/Microsoft.Gen.MetadataExtractor/GoldenReports/MeterAttributedWithXmlDescriptions.json
Outdated
Show resolved
Hide resolved
| } | ||
| ] | ||
|
|
There was a problem hiding this comment.
I will revisit the metric report emitter and see what is going on exactly inside.
There was a problem hiding this comment.
the indent char is 4 from the EmitterBase
it goes like this :
(root) 4 -> (MetricReport ) -> 8 ([) -> 12 -> ({metricClass.RootNamespace}) ->16
| OutLn(" {"); | ||
|
|
||
| OutLn($" \"MetricName\": \"{metricMethod.MetricName.Replace("\\", "\\\\").Replace("\"", "\\\"")}\","); | ||
| OutLn($" \"MetricName\": \"{metricMethod.MetricName.Replace("\\", "\\\\").Replace("\"", "\\\"")}\","); |
There was a problem hiding this comment.
| OutLn($" \"MetricName\": \"{metricMethod.MetricName.Replace("\\", "\\\\").Replace("\"", "\\\"")}\","); | |
| OutIndent(); | |
| OutLn($"\"MetricName\": \"{metricMethod.MetricName.Replace("\\", "\\\\").Replace("\"", "\\\"")}\","); |
ditto to remove spaces in OutLn following this indentation.
| Indent(); | ||
| OutLn("{"); |
There was a problem hiding this comment.
| Indent(); | |
| OutLn("{"); | |
| OutOpenBrace(); |
ditto for any other Indent / Outln("{"} calls
| } | ||
|
|
||
| Out(" }"); | ||
| OutLn("}"); |
There was a problem hiding this comment.
| OutLn("}"); | |
| OutCloseBrace(); |
| OutLn("{"); | ||
| Indent(); |
There was a problem hiding this comment.
| OutLn("{"); | |
| Indent(); | |
| OutOpenBrace(); |
| Indent(); | ||
| OutLn($"\"{metricClass.RootNamespace}\":"); | ||
|
|
||
| if (metricClass.Methods.Length > 0) | ||
| { | ||
| OutLn(" ["); | ||
| Indent(); | ||
| OutLn("["); | ||
|
|
||
| for (int j = 0; j < metricClass.Methods.Length; j++) | ||
| { | ||
| var metricMethod = metricClass.Methods[j]; | ||
| Indent(); | ||
| ReportedMetricMethod metricMethod = metricClass.Methods[j]; | ||
| bool isLastReportedMetricMethod = (j == metricClass.Methods.Length - 1); | ||
|
|
||
| GenMetricMethodDefinition(metricMethod, cancellationToken); | ||
| GenMetricMethodDefinition(metricMethod, isLastReportedMetricMethod, cancellationToken); | ||
|
|
||
| if (j < metricClass.Methods.Length - 1) | ||
| if (!isLastReportedMetricMethod) | ||
| { | ||
| Out(","); | ||
| OutLn(); | ||
| } | ||
|
|
||
| OutLn(); | ||
| Unindent(); | ||
| } | ||
|
|
||
| OutLn(" ]"); | ||
| Unindent(); | ||
| OutLn("]"); | ||
| Unindent(); | ||
| } | ||
|
|
||
| Out(" }"); | ||
| OutLn("}"); |
There was a problem hiding this comment.
For consistency, consider how ComplianceReportEmitter uses OutArray. Although it's not in EmitterBase, so we could introduce a new EmiterJsonBase that inherits EmitterBase, and includes these additional JSON related helpers from ComplianceReportEmitter.
| @@ -13,70 +13,94 @@ namespace Microsoft.Gen.MetricsReports; | |||
|
|
|||
| internal sealed class MetricDefinitionEmitter : EmitterBase | |||
| { | |||
| private const int DimensionsIndentLevel = 2; | |||
There was a problem hiding this comment.
This feels unnecessary if indentation levels are handled correctly.
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.
Before :
After :
the test report has been changed to be aligned with the new formatting.
here are the unit tests :
Microsoft Reviewers: Open in CodeFlow