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

Improving the log of CombineTargetFrameworkInfoProperties fails with not valid RootElementName #9002

Merged
merged 3 commits into from Jul 17, 2023

Conversation

JaynieBai
Copy link
Member

Fixes #8320

Context

CombineTargetFrameworkInfoProperties) is not handling null case of RootElementName. And empty RootElementName when UseAttributeForTargetFrameworkInfoPropertyNames is false.

Changes Made

Add the verification with the name of the parameter.

Testing

RootElementNameNotValid()

Notes

@JaynieBai JaynieBai marked this pull request as ready for review July 10, 2023 10:07
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

While the MSBuild engine handles exceptions thrown from tasks and provides a generic error message, it's almost always better to log an explicit error from the task instead of throwing an exception. It's easier to understand, you can provide more information about the specific failure, and it can get a unique error code.

Can you please rewrite to use that approach?

@JaynieBai
Copy link
Member Author

While the MSBuild engine handles exceptions thrown from tasks and provides a generic error message, it's almost always better to log an explicit error from the task instead of throwing an exception. It's easier to understand, you can provide more information about the specific failure, and it can get a unique error code.

Can you please rewrite to use that approa

While the MSBuild engine handles exceptions thrown from tasks and provides a generic error message, it's almost always better to log an explicit error from the task instead of throwing an exception. It's easier to understand, you can provide more information about the specific failure, and it can get a unique error code.

Can you please rewrite to use that approach?

Done

Copy link
Member

@JanKrivanek JanKrivanek 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!

src/Tasks/Resources/Strings.resx Outdated Show resolved Hide resolved
@JaynieBai JaynieBai merged commit 91a3521 into main Jul 17, 2023
8 checks passed
@JaynieBai JaynieBai deleted the jennybai/issue8320 branch July 17, 2023 05:45
YuliiaKovalova pushed a commit to YuliiaKovalova/msbuild that referenced this pull request Jul 18, 2023
…not valid RootElementName (dotnet#9002)

Fixes dotnet#8320

Context
CombineTargetFrameworkInfoProperties) is not handling null case of RootElementName. And empty RootElementName when UseAttributeForTargetFrameworkInfoPropertyNames is false.

Changes Made
Add the verification with the name of the parameter.

Testing
RootElementNameNotValid()

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

CombineTargetFrameworkInfoProperties fails with ArgumentNullException
3 participants