Skip to content

Conversation

@JamesWTruher
Copy link
Collaborator

@JamesWTruher JamesWTruher commented Feb 8, 2017

#1084

This does not support providing a specific version to run, but
like most other *nix commands, -version will now return the version
of the PowerShell Engine. 'powershell' is prepended to the output to
match other *nix commands. We are using gitcommitid which includes more
info about the build.

@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Feb 8, 2017
@SteveL-MSFT SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Feb 8, 2017
@joeyaiello joeyaiello added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Feb 9, 2017
@joeyaiello
Copy link
Contributor

@PowerShell/powershell-committee is good with all of this. 👍

Copy link
Collaborator

@vors vors Feb 9, 2017

Choose a reason for hiding this comment

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

redundant extra-line before else #Resolved

Copy link
Collaborator

@vors vors Feb 9, 2017

Choose a reason for hiding this comment

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

why not simple { get; private set; } #WontFix

Copy link
Member

@daxian-dbw daxian-dbw Feb 9, 2017

Choose a reason for hiding this comment

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

I think it's fine to follow the existing code pattern here. We can convert the style using a tool later. #ByDesign

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to match the current code style


In reply to: 100213197 [](ancestors = 100213197)

Copy link
Collaborator

@vors vors Feb 9, 2017

Choose a reason for hiding this comment

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

I wonder how it's going to interact with existing in Windows PowerShell
powershell -version 2.0

Since we are stopping to build Windows powershell, and core edition doesn't have any -version 2.0 support, it's probably okey. #ByDesign

Copy link
Member

@SteveL-MSFT SteveL-MSFT Feb 9, 2017

Choose a reason for hiding this comment

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

@vors, the @PowerShell/powershell-committee discussed this and that is pretty much the thinking: -version for Windows PowerShell really only supported 2.0 and we're moving away from that. Aligning with Linux convention makes sense. PSCore supports side-by-side intrinsically and using file paths to launch different versions is completely acceptable. #ByDesign

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore we can always do something like powershell -version:2 if people really demand that functionality in the future (and actually @lzybkr and @BrucePay agreed that it would even be acceptable to do powershell -version 2 as it's highly unlikely that anyone wants to echo one int when they shell out to PowerShell).

Copy link
Member

@daxian-dbw daxian-dbw Feb 9, 2017

Choose a reason for hiding this comment

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

Why are you setting _commandLineCommand here? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

vestige of earlier implementation - removed


In reply to: 100401276 [](ancestors = 100401276)

Copy link
Member

@daxian-dbw daxian-dbw Feb 9, 2017

Choose a reason for hiding this comment

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

I recommend to use PSVersionInfo.GetPSVersionTable()["GitCommitId"], or even better, add an internal static property GitCommitId right after the PSVersion property at here, and then use PSVersionInfo.GitCommitId here.

Reason: PSVersionInfo has a type initializer, which calls GetCommitInfo() and store the value in a hashtable. the current implementation of GetCommitInfo() reads git commit information from a file, which is slow and this implementation is subject to change (this method may be gone in near future -- we want to generate code at build time to embed the git commit id info, so as to avoid reading a file at startup time). Since the type initializer must run the first time you access a type, it's a waste to call GetCommitInfo() here to read the file again. #Resolved

Copy link
Member

@daxian-dbw daxian-dbw Feb 9, 2017

Choose a reason for hiding this comment

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

There is an extra blank line here. #Resolved

Copy link
Member

@daxian-dbw daxian-dbw Feb 9, 2017

Choose a reason for hiding this comment

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

what would happen if I have powershell -noprofile -noexit -version? #WontFix

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 would report the version and exit - I can add a test for that explicitly if you like. Rather than testing all of the permutations for the parameters, I chose a representative single case


In reply to: 100405816 [](ancestors = 100405816)

@daxian-dbw daxian-dbw self-assigned this Feb 9, 2017
@daxian-dbw daxian-dbw added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Feb 9, 2017
…ell#1084)

This does not support providing a specific version to run, but
like most other *nix commands, -version will now return the version
of the PowerShell Engine. 'powershell' is prepended to the output to
match other *nix commands. We are using gitcommitid which includes more
info about the build.
@PowerShellTeam PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Feb 17, 2017
…on PSVersionInfo

Remove extraneous blank lines in files
remove line which set the commandline when retrieving the version as it is unneeded
@JamesWTruher JamesWTruher force-pushed the jameswtruher/ShowVersion branch from 39e0991 to 96abb8f Compare February 17, 2017 23:12
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@daxian-dbw daxian-dbw merged commit 9e27f4a into PowerShell:master Feb 23, 2017
@iSazonov iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 2017
@JamesWTruher JamesWTruher deleted the jameswtruher/ShowVersion branch September 23, 2023 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants