Skip to content

Refactor build logic to avoid saving modules in subfolders #979

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

Merged
merged 3 commits into from
Jun 28, 2019
Merged

Refactor build logic to avoid saving modules in subfolders #979

merged 3 commits into from
Jun 28, 2019

Conversation

ghvanderweg
Copy link

Fixes PowerShell/vscode-powershell#2027

This PR refactors part of the build logic in PowerShellEditorServices.build.ps1, to avoid saving the modules defined in modules.json in versioned subfolders. This allows the modules (currently Plaster and PSScriptAnalyzer) to be loaded when running on PowerShell 3 and 4.

It also moves the modules out of a subfolder in case they're already present and saved from a previous build (wasn't sure if I could assume a clean build).

The build script itself also works in PowerShell 3 and up.

Build and extension both tested on Windows PowerShell 3 and 5.1 & PowerShell 7-preview.

@ghvanderweg ghvanderweg requested a review from rjmholt as a code owner June 20, 2019 19:40
@msftclas
Copy link

msftclas commented Jun 20, 2019

CLA assistant check
All CLA requirements met.

@ghvanderweg
Copy link
Author

Not sure why the macOS CI failed, but I don't think it was my change.

@PowerShell PowerShell deleted a comment Jun 21, 2019
@ghvanderweg
Copy link
Author

Should I remove/change the calls to Write-Host while I'm at it? They were already there but Codacy doesn't like it.

@TylerLeonhardt
Copy link
Member

No need. They are valid for this usecase. Just ignore Codacy

@corbob
Copy link
Contributor

corbob commented Jun 21, 2019

No need. They are valid for this usecase. Just ignore Codacy

Does Codacy have a way to tell it to ignore things like PSSA does? Would it be worth (not necessarily in this PR) adding comments or directives indicating that we're aware of the rule "violation" and provide justification?

@TylerLeonhardt
Copy link
Member

@corbob I think so? @adityapatwardhan do you know how we could configure Codacy with the Rules we care about?

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM awesome work @ghvanderweg! 🎉

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Jun 25, 2019

@corbob

So basically we need to contribute to https://github.com/adityapatwardhan/ps-codacy

to allow for settings file support, then @adityapatwardhan can publish a new version and codacy will automagically pick it up.

Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your work @ghvanderweg


Get-ChildItem -Path $moduleSubfolder -Recurse -Force | Move-Item -Destination $moduleInstallPath -Force

if ($null -eq (Get-ChildItem -Path $moduleSubfolder -Recurse -Force))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd turn this into if ($null -ne (...)) { throw $message } and then proceed with the golden path operation

Copy link
Author

Choose a reason for hiding this comment

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

Good call. I've changed it.

@PowerShell PowerShell deleted a comment Jun 26, 2019
@PowerShell PowerShell deleted a comment Jun 26, 2019
@PowerShell PowerShell deleted a comment Jun 26, 2019
@rjmholt rjmholt merged commit d049ce7 into PowerShell:legacy/1.x Jun 28, 2019
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.

6 participants