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

Write an error for powershell -version <any value> #4931

Merged
merged 5 commits into from
Sep 29, 2017

Conversation

iSazonov
Copy link
Collaborator

Fix #4834

@@ -272,4 +272,7 @@ EXAMPLES
<data name="InvalidArgument" xml:space="preserve">
<value>Invalid argument '{0}', did you mean:</value>
</data>
<data name="DeprecatedVersionParameter" xml:space="preserve">
<value>Old functionality of '-version 2.0' is deprecated. Now '-version' parameter shows current PowerShell version. </value>
Copy link
Member

Choose a reason for hiding this comment

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

I think we should change this error message to:

Previous usage of '-version {0}' is not supported.  '-version' currently only supports returning the current PowerShell version.

cc @joeyaiello

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the error message - please review.

++i;
if (i < args.Length)
{
if (LanguagePrimitives.TryConvertTo<int>(args[i], out int verNumber) && verNumber == 2)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should currently error if any value is passed to -v

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now we write the error and then show current version.
Please clarify your request.

Copy link
Member

Choose a reason for hiding this comment

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

Aren't you only writing error if -v 2? Should also error if -v 3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. Will add the error for -v 3 too.

Copy link
Member

Choose a reason for hiding this comment

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

We should error on any value passed to -v. In some future time if we decide to support launching different installed versions of PSCore6, we can decide if we want to overload -v or even introduce a new parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently we ignore all values after -v. The docs says the same: accept only 2.0 or 3.0. If anybody use the parameter the breaking change is related only to 2.0 or 3.0 values - we should write an error for the cases only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you comment #4836 - I am ready to implement enumeration of installed versioms.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we currently ignore all values after -v, then it seems logical to write error on any value passed to -v saying that parameter doesn't have any effect.
Having a parameter that quietly does nothing doesn't look good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is an enhancement. It make sense. Thanks!

Done.

Copy link
Contributor

@anmenaga anmenaga left a comment

Choose a reason for hiding this comment

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

Please add tests.

++i;
if (i < args.Length)
{
if (LanguagePrimitives.TryConvertTo<int>(args[i], out int verNumber) && (verNumber == 2 || verNumber == 3))
Copy link
Member

Choose a reason for hiding this comment

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

I think we should error on any value passed to -v as part of this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@iSazonov iSazonov changed the title Write an error for powershell -version 2 Write an error for powershell -version <any value> Sep 29, 2017
@iSazonov iSazonov merged commit 757c6b5 into PowerShell:master Sep 29, 2017
@iSazonov iSazonov deleted the version2error branch September 29, 2017 19:20
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.

None yet

3 participants