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

Fix that module can be found by not imported? #351

Merged
merged 2 commits into from
Jan 4, 2017
Merged

Fix that module can be found by not imported? #351

merged 2 commits into from
Jan 4, 2017

Conversation

dahlbyk
Copy link
Owner

@dahlbyk dahlbyk commented Jan 3, 2017

Fix #350 and #322 (comment), I think?

I can't explain why Get-Module posh-git would find a module but Import-Module posh-git wouldn't find the same. I'm wondering if the Get-Module result should be filtered through Select-Object -First 1 and maybe sorted by version or something?

@dahlbyk dahlbyk requested a review from rkeithhill January 3, 2017 23:27
@dahlbyk dahlbyk added this to the v0.7 milestone Jan 3, 2017
@rkeithhill
Copy link
Collaborator

I'm wondering if the Get-Module result should be filtered through Select-Object -First 1 and maybe sorted by version or something?

But if Get-Module finds anything, it should load the latest version unless one of the Version parameters are specified. In this case, they are not.

Anyway, your fix should eliminate the possibility of error but I would Sort by version -desc and Select the first module.

We should also move the prompt function definition before we import the module so that the import will detect a custom prompt function is in use so it won't set a new prompt that get's clobbered later by the prompt function appearing after the import. It's not that this won't work, it is a slight perf improvement.

Copy link
Collaborator

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

See my other comment about using Sort / Select. Also consider moving prompt def before the import statement if we decide to go with PR #349

@dahlbyk
Copy link
Owner Author

dahlbyk commented Jan 4, 2017

We should also move the prompt function definition before we import the module so that the import will detect a custom prompt function is in use so it won't set a new prompt that get's clobbered later by the prompt function appearing after the import. It's not that this won't work, it is a slight perf improvement.

I'm inclined to drop the prompt from profile.example.ps1 altogether as part of #349.

I would Sort by version -desc and Select the first module.

Done. Good?

Copy link
Collaborator

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

LGTM

@dahlbyk dahlbyk merged commit 1f459b6 into master Jan 4, 2017
@dahlbyk dahlbyk deleted the import-fix branch January 4, 2017 07:32
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.

2 participants