Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
iSazonov merged 5 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:version2erroriSazonov/PowerShell:version2errorCopy head branch name to clipboard
Sep 29, 2017
Merged

Write an error for powershell -version <any value>#4931
iSazonov merged 5 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:version2erroriSazonov/PowerShell:version2errorCopy head branch name to clipboard

Conversation

@iSazonov

Copy link
Copy Markdown
Collaborator

Fix #4834

<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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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.

@anmenaga anmenaga left a comment

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.