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

MissingRestorePropertyError #7280

Merged
merged 11 commits into from
Jan 24, 2022
Merged

MissingRestorePropertyError #7280

merged 11 commits into from
Jan 24, 2022

Conversation

jrdodds
Copy link
Contributor

@jrdodds jrdodds commented Jan 12, 2022

Fixes #7218

Context

MissingRestorePropertyError doesn't exist. The /rp switch accepts properties to be used during a Restore target. The /p switch accepts properties. The InvalidPropertyError is used by both switches. It appears the intent may have been to also share the MissingPropertyError.

Changes Made

Edit for ParameterizedSwitch.RestoreProperty to use MissingPropertyError in place of MissingRestorePropertyError.
Edit to unit test PropertySwitchIdentificationTests to also test the ParameterizedSwitch.RestoreProperty.

Testing

Tested locally built msbuild.exe. An exception is no longer thrown.

Notes

parameterizedSwitch.ShouldBe(CommandLineSwitches.ParameterizedSwitch.RestoreProperty);
duplicateSwitchErrorMessage.ShouldBeNull();
multipleParametersAllowed.ShouldBeTrue();
missingParametersErrorMessage.ShouldNotBeNull();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe specify that this should be the property error message? You would have to get it out of strings with ResourceUtilities.FormatResourceString or something to ensure it works on any locale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a set of these unit tests which are all effectively testing the CommandLineSwitches.IsParameterizedSwitch() method. That method doesn't retrieve the resource string. It pulls a ParameterizedSwitchInfo object from the s_parameterizedSwitchesMap array.

The RestorePropertySwitchIdentificationTests test could be modified to check that the value of missingParametersErrorMessage is "MissingPropertyError".

Should all of the unit tests with missingParametersErrorMessage.ShouldNotBeNull(); be updated accordingly? (I'm happy to make the changes but I don't want to presume.)

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good to me. As far as changing any of the other tests, it sounds reasonable to me, but it's less relevant to the main point of this PR, so if you decide to change them all, I'd put that in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unit test has been updated. Thanks

@Forgind
Copy link
Member

Forgind commented Jan 24, 2022

Copied from issue:

Examples

Property switch missing property

msbuild test.proj /p

generates error

MSBUILD : error MSB1005: Specify a property and its value.
Switch: /p

RestoreProperty switch missing property

msbuild test.proj /rp

generates error

MSBUILD : error MSB1005: Specify a property and its value.
Switch: /rp

RestoreProperty and Property switches both missing property

msbuild test.proj /rp /p

generates error

MSBUILD : error MSB1005: Specify a property and its value.
Switch: /rp

If the command line has multiple errors, only one error is reported.

@rainersigwald
Copy link
Member

msbuild test.proj /rp

generates error

MSBUILD : error MSB1005: Specify a property and its value.
Switch: /rp

This looks fine since it still makes clear that it's the /rp switch that is in error.

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 24, 2022
@ladipro ladipro merged commit afb0dc9 into dotnet:main Jan 24, 2022
@jrdodds jrdodds deleted the MissingRestorePropertyError branch January 25, 2022 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MissingRestorePropertyError doesn't exist
4 participants