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

Remove PowerShell v2 support. #427

Merged
merged 7 commits into from Feb 20, 2017
Merged

Conversation

rkeithhill
Copy link
Collaborator

@rkeithhill rkeithhill commented Feb 18, 2017

Also doing some miscellaneous cleanup (remove Invoke-NullCoalescing, remove testDebugHarness.ps1 in favor of using the Pester Tests debug configuration, rename CheckVersion.ps1 to CheckRequirements.ps1).

Checkout the version number (1.0.0-pre00) in the chocolatey/poshgit.nuspec file. Not sure which version of NuGet is being used but if v2, I think the prelease string uses a simple lexicographic sort so that pre10 would sort below pre9. I suspect 1.0 could have more than 10 prerelease choco drops. I hope not more than 100 though. :-)

cc #163

@rkeithhill rkeithhill added this to the v1.0 milestone Feb 18, 2017
Also doing some miscellaneous cleanup (remove Invoke-NullCoalescing, remove testDebugHarness.ps1 in favor of using the Pester Tests debug configuration, rename CheckVersion.ps1 to CheckRequirements.ps1).
Copy link
Owner

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

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

I think we can remove a few more exported functions:

  • Enable-GitColors
  • Get-AliasPattern
  • Get-GitBranch (data exposed through Get-GitStatus)
  • Write-Prompt

appveyor.yml Outdated
@@ -3,7 +3,7 @@ os:

branches:
only:
- master
Copy link
Owner

Choose a reason for hiding this comment

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

We probably want to keep both master and develop. That change may also need to find its way into master (the default branch) for it to take effect (not certain about that).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally there would be a way in appveyor.yml to specify the "current target branch". So if we put in both master and develop will that attempt to build a PR meant for develop on master also? If so, that would likely fail. Then again, I'm a total AppVeyor n00b.

Copy link
Owner

Choose a reason for hiding this comment

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

It just means any change in master or develop will be built. We will probably want to add a badge for develop, too.

PRs are built regardless of target; this setting is (ideally) only for merged PRs.


# Version number of this module.
ModuleVersion = '0.7.0'
ModuleVersion = '1.0.0'
Copy link
Owner

Choose a reason for hiding this comment

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

Really wish this supported a prerelease suffix. I'd like to be able to differentiate between folks using a PowerShell Gallery version and this branch. Maybe use 1.0.0.0 and then switch to 1.0.0 when the time comes to release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

'1.0.0' comes out as version 1.0.0.-1 so the order between release and pre is wrong. :-(
We can add a field to the module manifest private data to indicate a PrereleaseVersion string that we could retrieve with (gmo posh-git).PrivateData.PSData.PrereleaseVersion.

Copy link
Owner

Choose a reason for hiding this comment

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

I am mostly concerned with the issue template version script showing a difference between loading from develop/master and from Chocolatey or the Gallery.

An alternative would be to include the posh-git module's path, perhaps with a replacement of $Home with ~?

Copy link
Owner

Choose a reason for hiding this comment

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

'1.0.0' comes out as version 1.0.0.-1 so the order between release and pre is wrong.

It may actually be desirable for a local version (think fork with personalized features) to win out over an official version (e.g. inherited from GitHub Desktop). Maybe? Of course the personalized fork could just bump the version...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The way I handle that scenario is to import by path e.g:

Import-Module $Home\GitHub\dahlbyk\posh-git\src\posh-git.psd1

Development is in the works for PowerShellGet/PSGallery to support prereleases. I'm hoping this problem will get solved in the coming months. PowerShell/PowerShell-RFC#42

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, but doing that if posh-git has already been loaded, e.g. in the GitHub Desktop shell, leaves you with two posh-git modules loaded. If one of those is 0.7.0 and the other is a personal fork that also reports 0.7.0, I have to check path to differentiate. Thus my suggestion of the extra .0 as a hint that this isn't an official version.

That said, also including PrereleaseVersion makes sense… add that to the issue template script?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. But I'll update the issue template file on master in a separate PR. Also, regarding those extra commands to not export. I'll remove those. I'm guilty of adding Get-GitBranch for testing purposes but I think Pesters's InModuleScope will get me access to this function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What if we leave in Write-Prompt to abstract the difference between using Write-Host and ANSI escape sequences. Folks customizing their own prompt function might find that useful.

@@ -50,7 +49,7 @@ CmdletsToExport = @()
VariablesToExport = @()

# Aliases to export from this module
AliasesToExport = @('??')
AliasesToExport = @()
Copy link
Owner

Choose a reason for hiding this comment

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

Remove until we stumble on an alias we want to export (unlikely)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

An empty array helps with overall system performance.

If you omit the entry, PowerShell does more expensive work to see if you meant to export any aliases.

@@ -3,7 +3,7 @@
<metadata>
<id>poshgit</id>
<title>posh-git</title>
<version>0.7.0</version>
<version>1.0.0-pre00</version>
Copy link
Owner

Choose a reason for hiding this comment

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

👍

rkeithhill and others added 4 commits February 19, 2017 19:03
Also add PrereleaseVersion as module private data property.  Might come in handy.

I'll update the issue template on master and then submit a PR to merge master to develop.
@dahlbyk dahlbyk merged commit 05fc072 into develop Feb 20, 2017
@dahlbyk dahlbyk deleted the rkeithhill/remove-PS2.0-support branch February 20, 2017 06:03
@dahlbyk
Copy link
Owner

dahlbyk commented Feb 20, 2017

So build is green here, but failed on develop. @rkeithhill can you take a look?

@rkeithhill
Copy link
Collaborator Author

Yup. Could be the switch to using InModuleScope to access the Get-GitBranch function (as opposed to exporting it).

@dahlbyk
Copy link
Owner

dahlbyk commented Feb 20, 2017

Could be the switch to using InModuleScope to access the Get-GitBranch function (as opposed to exporting it).

I wouldn't expect that to have passed in the PR build. My guess is it's due to building on develop instead of master?

@rkeithhill
Copy link
Collaborator Author

rkeithhill commented Feb 20, 2017

Was just looking more closely at the error messages. It doesn't like creating #develop. I wouldn't think that would interfere with the existing develop branch.

@rkeithhill
Copy link
Collaborator Author

I can run these tests locally on both master and develop and they all pass. This must have something to do with how AppVeyor sets up the build/test environment. For instance, when it uses develop, I see failures like this:

        Expected exactly: {master}

        But was: {}

Is it fetching develop only and not master as well?

@dahlbyk
Copy link
Owner

dahlbyk commented Feb 20, 2017

Context 'Fetch/Push/Pull TabExpansion Tests' doesn't use a test repo; it's using the posh-git repo. I get the same set of failures if I delete my local master branch. PR incoming.

@rkeithhill
Copy link
Collaborator Author

Yeah, my bad. Thanks for the help on this!

@dahlbyk
Copy link
Owner

dahlbyk commented Feb 20, 2017

PR opened into master. I'll open another PR from master into develop when this is merged.

@dahlbyk dahlbyk mentioned this pull request Dec 13, 2022
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