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

Pretty formatted XML #111

Merged
merged 8 commits into from
May 21, 2024
Merged

Pretty formatted XML #111

merged 8 commits into from
May 21, 2024

Conversation

omus
Copy link
Member

@omus omus commented May 17, 2024

After working on this package for a while I think the reference tests would be better if we used pretty formatted XML as it makes it more obvious what's wrong in the reference test diff. The counter argument to this is that this re-order the attributes from what users get out of the box without pretty printing.

Depends on:

@omus omus requested a review from oxinabox May 17, 2024 19:57
@oxinabox
Copy link
Member

Can we change to always pretty printing? That would make these tests consistent with the actual behavour users see.
And would also generally be a improvement for most users i think.
It would be good to get this in as part of 1.0

That would be changing the write to be a open + prettyprint

write($(repr(logfilename)), report(TestReports.flatten_results!(ts)))

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Approving this, as i don't think you need further feedback beyond the comment i gave above before this is mergable

@omus
Copy link
Member Author

omus commented May 21, 2024

Can we change to always pretty printing? That would make these tests consistent with the actual behavour users see. And would also generally be a improvement for most users i think. It would be good to get this in as part of 1.0

That would be changing the write to be a open + prettyprint

write($(repr(logfilename)), report(TestReports.flatten_results!(ts)))

Sounds good. I do like have the option of having compact printing so maybe I'll look into having an keyword for this.

Base automatically changed from cv/test-properties to master May 21, 2024 18:21
@omus
Copy link
Member Author

omus commented May 21, 2024

I do like have the option of having compact printing so maybe I'll look into having an keyword for this.

I still think this is a good idea but is more additional work I don't want to take on at this time. As the users can use report directly and render the XML structure however they want I think this is less of an issue than I first thought.

@omus omus changed the title Use pretty formatted XML in reference tests Pretty formatted XML May 21, 2024
@omus omus merged commit 16f8d25 into master May 21, 2024
10 checks passed
@omus omus deleted the cv/pretty-reference-tests branch May 21, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants