Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweak MetaData Report format for better readability #5929

Open
wants to merge 76 commits into
base: main
Choose a base branch
from

Conversation

IbrahimMNada
Copy link
Contributor

@IbrahimMNada IbrahimMNada commented Feb 19, 2025

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 GeneratorTests class in the namespace Microsoft.Gen.MetadataExtractor.Unit.Tests; .
I introduced new method called NormalizeEscapes, which will only escape "\r\n" to neglect the gap between
Windows & Linux, I hope this addition is acceptable.

Before :
image

After :
image

the test report has been changed to be aligned with the new formatting.
here are the unit tests :

image
Microsoft Reviewers: Open in CodeFlow

@IbrahimMNada
Copy link
Contributor Author

the checks failed because of something related to hybrid caching

@IbrahimMNada IbrahimMNada requested a review from a team as a code owner February 19, 2025 09:22
@dotnet-comment-bot
Copy link
Collaborator

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Gen.MetadataExtractor 57 70
Microsoft.Extensions.Caching.Hybrid 82 87

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=956346&view=codecoverage-tab

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.AI Line 89 88.54 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Gen.MetadataExtractor 57 70
Microsoft.Extensions.AI.Abstractions 82 83

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=957466&view=codecoverage-tab

@RussKie
Copy link
Member

RussKie commented Feb 20, 2025

Project Expected Actual
Microsoft.Gen.MetadataExtractor 57 70

Could you please also bump this threshold?

@RussKie RussKie added waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. area-telemetry labels Feb 20, 2025
@IbrahimMNada
Copy link
Contributor Author

@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.
same goes for tmp and var.

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 dotnet-policy-service bot removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Feb 20, 2025
@IbrahimMNada IbrahimMNada requested a review from a team as a code owner February 20, 2025 09:09
@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.AI Line 89 88.76 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Extensions.AI.Abstractions 82 83
Microsoft.Gen.MetadataExtractor 57 70

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=957592&view=codecoverage-tab

@IbrahimMNada
Copy link
Contributor Author

IbrahimMNada commented Mar 12, 2025

I have excluded JsonEmitter from projects that does not use it, which led the WarningCheks to succussed.
and ran all tests for the whole project to double check that the changes did not break any thing, all is working as expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the formatting still has inconsistencies, can we fix that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please provide me your thoughts on the justifications bellow ?
if you have any further insights please share them with me

Comment on lines +37 to +41
[
{
"TestClasses":
[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example here, arrays should start the bracket on the same line as the property, and containing objects indented only one level

Suggested change
"MetricReport":
[
{
"TestClasses":
[
"MetricReport": [
{
"TestClasses": [

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason for this is that all the generators start with an empty line.
the MetaDataGenerator utilizes the MetricsReportsGenerator which will start with a empty line by default.
if you look at ComplianceReports it will be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can for example introduce a new flag to decide whither to keep the newline or not

Comment on lines 45 to 48
{
"Dimension1": "Dimension1 description."
}
Copy link
Contributor

@dariusclay dariusclay Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here it has an extra indentation, and doesn't follow usual JSON formatting for object properties

Suggested change
"Dimensions":
{
"Dimension1": "Dimension1 description."
}
"Dimensions": {
"Dimension1": "Dimension1 description."
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have removed the indentations. However putting the open brace up above will require me to change also on Compliance because every json object is opened on a separated line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mind changing the compliance ? it remained untouched

@IbrahimMNada IbrahimMNada requested a review from a team as a code owner March 14, 2025 11:27
@dariusclay dariusclay self-requested a review March 18, 2025 15:07
@IbrahimMNada
Copy link
Contributor Author

is there anything further needs to be done ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants