-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
made CircularDependencyExcpetion class public #5988
Conversation
@tflynt91 could you provide context for this change in the description? |
|
Will do! |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last blocker is to sign the CLA. Otherwise, LGTM!
great thanks! I believe I signed it but maybe the status is still pending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a couple of nitpicks to address.
I don't know why there are two comments about the CLA but one of them and the check are happy so I believe you signed it :)
@@ -31,7 +31,7 @@ internal CircularDependencyException(string message) | |||
/// <summary> | |||
/// Constructor for deserialization. | |||
/// </summary> | |||
protected CircularDependencyException(SerializationInfo info, StreamingContext context) | |||
public CircularDependencyException(SerializationInfo info, StreamingContext context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change necessary? I think it's ok for this ideally-unused constructor to continue to be protected.
@@ -1,5 +1,12 @@ | |||
// Copyright (c) Microsoft. All rights reserved. | |||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | |||
namespace Microsoft.Build.BackEnd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't exposed this namespace before, so I think we should move the exception into the Microsoft.Build.Exceptions
namespace where the others are:
msbuild/ref/Microsoft.Build/net/Microsoft.Build.cs
Lines 887 to 890 in dcf8be3
namespace Microsoft.Build.Exceptions | |
{ | |
public partial class BuildAbortedException : System.Exception | |
{ |
Can you change the namespace in CircularDependencyException.cs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good! I think keeping it protected was causing some error preventing it from building. The build worked on my end, I think the namespace change might resolve that, so I changed it back to protected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thank you!
Closes #5811