-
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
Use a task in _CheckForInvalidConfigurationAndPlatform #3105
Conversation
This gives us the ability to localize messages that are logged in this target.
/// <summary> | ||
/// Verifies some critical properties like OutputPath are set. This runs as part of the _CheckForInvalidConfigurationAndPlatform target. | ||
/// </summary> | ||
public class CheckConfigurationAndPlatform : TaskExtension |
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.
Rather than make a specialized task, why not make general localized Error and Warning tasks (maybe subclass the existing ones if that fits) that take in a resource name?
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.
I started with that but couldn’t figure out a good way to specify the assembly and resource. You would need one Error task per assembly that contained resources right? And if you specify an assembly in XML, it would need to be a path. And then the way you call it with format args would be interesting. Any ideas?
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.
Maybe just always have it be a resource in the Tasks resources, and just pass in a name? It'd only work for us, but it'd be a step in the right direction, I think.
Passing in things for the format string is definitely interesting. Maybe pass in an item and have metadata like Field1 Field2? That's not exactly elegant :(
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.
See https://github.com/dotnet/sdk/blob/master/src/Tasks/Common/MessageBase.cs and https://github.com/dotnet/sdk/blob/master/src/Tasks/Common/NETSdkError.cs for how we handle this in dotnet/sdk
@jeffkl do you want to update this for what Daniel suggested or close the issue? I'd rather not take this as-is but rather introduce a generic task (generic for us to use). |
@AndyGerlicher I barely had time to code this up as it was. The only thing missing is unifying our |
I'd rather just fix HasTrailingSlash, if we want a whole new task that accepts a resource name and arguments that'll be a different PR. |
This gives us the ability to localize messages that are logged in this target.
Related to #3059