Skip to content

Improve ModulePath calculation and refactor related method names - #2265

Merged
Mike Richmond (mirichmo) merged 3 commits into
PowerShell:masterfrom
daxian-dbw:improve-modulepath
Sep 15, 2016
Merged

Improve ModulePath calculation and refactor related method names#2265
Mike Richmond (mirichmo) merged 3 commits into
PowerShell:masterfrom
daxian-dbw:improve-modulepath

Conversation

@daxian-dbw

@daxian-dbw Dongbo Wang (daxian-dbw) commented Sep 14, 2016

Copy link
Copy Markdown
Member

Resolving #1744

The PR include following changes:

  1. Refactor method names related to module path calculation.
  2. Improve the logic that detects if a module path is PSHome module path derived from a different powershell core instance.
  3. Add more sxs module path tests.

@msftclas

Microsoft Contribution License Agreements (msftclas) commented Sep 14, 2016

Copy link
Copy Markdown

Hi Dongbo Wang (@daxian-dbw), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Dongbo Wang). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 14, 2016

Copy link
Copy Markdown
Member Author

Mike Richmond (@mirichmo) could you please review this PR?

mirichmo
Mike Richmond (mirichmo) suggested changes Sep 14, 2016
View reviewed changes

@mirichmo Mike Richmond (mirichmo) left a comment

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.

The code changes look good. I have one issue with the tests though.

Comment thread test/powershell/enginecore/ModulePath.Tests.ps1 Outdated

@mirichmo Mike Richmond (mirichmo) Sep 14, 2016

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.

The creation of the fake powershell.exe and deps file should be done in a BeforeEach and After each for this test. It should not be cleaned up in the next test. I suggest using a Context to limit the BeforeEach and AfterEach to this test.

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 14, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread src/System.Management.Automation/engine/InitialSessionState.cs Outdated

@mirichmo Mike Richmond (mirichmo) Sep 14, 2016

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.

Are all 4 commits necessary for the history, or can they be squashed?

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 14, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we should keep them separate commits, especially the one that rename the method names.

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 14, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have squashed test changes into one commit.

mirichmo
Mike Richmond (mirichmo) approved these changes Sep 15, 2016
View reviewed changes

@mirichmo Mike Richmond (mirichmo) left a comment

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.

LGTM

@mirichmo
Mike Richmond (mirichmo) merged commit d3a49c2 into PowerShell:master Sep 15, 2016
@daxian-dbw Dongbo Wang (daxian-dbw) mentioned this pull request Sep 15, 2016
Closed
3 of 4 tasks
@daxian-dbw
Dongbo Wang (daxian-dbw) deleted the improve-modulepath branch September 16, 2016 04:00
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@mirichmo
Merge pull request PowerShell#2265 from daxian-dbw/improve-modulepath
db8a509 
Improve ModulePath calculation and refactor related method names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@mirichmo Mike Richmond (mirichmo) mirichmo approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mirichmo Mike Richmond (mirichmo)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@daxian-dbw @msftclas @mirichmo