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

fix(.NET): Improve Collection of Errors string #728

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

texastony
Copy link
Contributor

@texastony texastony commented Dec 23, 2024

Issue #, if available:

Description of changes:
Collection of Errors is a frustrating error because it requires customer action
to completely serialize it into logs.

We can make a simple fix in .NET to improve the CX,
by always serializing the list into the Collection Of Error's
message.

We concatenate the given error message with
a serialization of all the nested errors.

See similar PR for MPL-Java: aws/aws-cryptographic-material-providers-library@9e195a1

Squash/merge commit message, if applicable:

fix(.NET): Improve Collection of Errors string 

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@texastony texastony changed the title fix(.NET): always include listed errors in Collection of Errs msg fix(.NET): Improve Collection of Errors string Dec 23, 2024
Patch files updated via:
```
gmake polymorph_dotnet DAFNY_VERSION=4.9.0 POLYMORPH_OPTIONS=--update-patch-files |& tee poly.log |& less -r +F
```
@texastony texastony force-pushed the tony/fix-collectionOfErrors branch from 0e05ae6 to 7705c2c Compare December 23, 2024 21:51
@texastony texastony marked this pull request as ready for review December 23, 2024 21:51
@texastony texastony requested a review from a team as a code owner December 23, 2024 21:51
public CollectionOfErrors(string message) : base(message) { this.list = new System.Collections.Generic.List<Exception>(); }
public CollectionOfErrors() : base("CollectionOfErrors") { this.list = new System.Collections.Generic.List<Exception>(); }

private static string ListAsString(List<Exception> list)
Copy link
Member

@rishav-karanjit rishav-karanjit Jan 2, 2025

Choose a reason for hiding this comment

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

Non blocking suggestion to maintain consistency in this class:

Suggested change
private static string ListAsString(List<Exception> list)
private static string ListAsString(System.Collections.Generic.List<Exception> list)

And remove the import and update dafny patch

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 am OK with this style divergence.
This is Beneration.
Ideally, we would go back and refactor Smithy-Dafny such that it always generates this.
I am not sure if/when we will do that,
as it may happen as part of other Smithy-Dafny feature development.

But this logic was written by a human;
it was not machine generated.

It is acceptable for it to appear as such;
it may even be preferred,
as it helps clarify what was benerated from what was generated.

Copy link
Member

@rishav-karanjit rishav-karanjit left a comment

Choose a reason for hiding this comment

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

I have a non blocking comment but without it as well this looks good to me.

@texastony texastony merged commit d66248b into mainline Jan 3, 2025
65 checks passed
@texastony texastony deleted the tony/fix-collectionOfErrors branch January 3, 2025 17:42
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