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 #1158

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

Conversation

texastony
Copy link
Contributor

Issue #, if available:

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: 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.

rishav-karanjit
rishav-karanjit previously approved these changes Jan 2, 2025
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.

Looks good to me

Got 2 new comments

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.

@rishav-karanjit rishav-karanjit dismissed their stale review January 2, 2025 19:46

Dismissing for this question

#1158 (review)

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

Choose a reason for hiding this comment

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

We are not importing System.Collections.Generic.List

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

Here and other places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@rishav-karanjit rishav-karanjit Jan 3, 2025

Choose a reason for hiding this comment

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

We are missing a import here. Thats why its a blocking comment.

For example: when I try to run this (below) it works but if I remove using System.Collections.Generic it will give me an error.

using System;
using System.Collections.Generic;

public class HelloWorld
{
    public static void Main(string[] args)
    {
        // Create a new List of strings
        List<string> names = new List<string>();

        // Add items to the list
        names.Add("Alice");
        names.Add("Bob");
        names.Add("Charlie");

        // Print all items in the list
        Console.WriteLine("All names:");
        foreach (string name in names)
        {
            Console.WriteLine(name);
        }
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Also, I guess the CI is passing because it only checks for output.Failure? but not output.error.TypeOfError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you nailed it.
I got it to work for one of the three PRs, and did not verify across all three repos.

Great catch.

@texastony
Copy link
Contributor Author

Are we skipping https://github.com/aws/aws-cryptographic-material-providers-library/blob/main/TestVectorsAwsCryptographicMaterialProviders/runtimes/net/Generated/KeyVectors/CollectionOfErrors.cs because its a test?

Yes. There is no customer impact, so I am not inclined to increase our Beneration scope.

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.

3 participants