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.

Sign up for GitHub

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

Jump to bottom

Error log the invalid the target name #9050

Merged
merged 7 commits into from
Jul 27, 2023
Merged

Error log the invalid the target name #9050

merged 7 commits into from
Jul 27, 2023

Conversation

JaynieBai
Copy link
Member

@JaynieBai JaynieBai commented Jul 19, 2023 •
edited
Loading

Fixes #9039

Context

It will have unhandled exception when specify a target with invalid character when building a solution.
For an individual project the error is a much cleaner, but it's still a bit misleading

Changes Made

Add invalid target name analysis in the method ProcessTargetSwitch

Testing

ProcessInvalidTargetSwitch()
image

Notes

@JaynieBai JaynieBai marked this pull request as ready for review July 19, 2023 10:02
rainersigwald
rainersigwald reviewed Jul 19, 2023
View reviewed changes
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.

Did you consider putting a IndexOfAny(XMakeElements.InvalidTargetNameCharacters) check in the target-specification code in xmake.cs? That might be slightly less invasive than this, while accomplishing very similar goals.

src/Build/Construction/ProjectTargetElement.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs Outdated Show resolved Hide resolved
@JaynieBai
Copy link
Member Author

Did you consider putting a IndexOfAny(XMakeElements.InvalidTargetNameCharacters) check in the target-specification code in xmake.cs? That might be slightly less invasive than this, while accomplishing very similar goals.

Thanks. Your suggestion way is better. I have updated that.

rainersigwald
rainersigwald reviewed Jul 20, 2023
View reviewed changes
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.

Looks good! Can you please add some test coverage in src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs?

@JaynieBai JaynieBai requested a review from JanKrivanek July 24, 2023 02:48
JanKrivanek
JanKrivanek approved these changes Jul 25, 2023
View reviewed changes
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

@JaynieBai JaynieBai merged commit 02379f2 into main Jul 27, 2023
@JaynieBai JaynieBai deleted the jennybai/issue9039 branch July 27, 2023 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JanKrivanek JanKrivanek JanKrivanek approved these changes

@rainersigwald rainersigwald rainersigwald approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Better and concise error message when project parameter has an invalid character like "."
3 participants
@JaynieBai @rainersigwald @JanKrivanek