-
Notifications
You must be signed in to change notification settings - Fork 498
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
Refactor build script and default to preview #1911
Refactor build script and default to preview #1911
Conversation
Codacy failures are just about |
…ode-powershell into refactor-build-script
@@ -119,20 +134,30 @@ task UpdateReadme { | |||
$readmeContent = Get-Content -Path $readmePath | |||
if (!($readmeContent -match "This is the PREVIEW version of the PowerShell extension")) { | |||
$readmeContent[0] = $newReadmeTop | |||
$readmeContent > $readmePath | |||
$readmeContent | Set-Content $readmePath -Encoding utf8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we're putting a BOM in right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm not sure what the default is for this... but I could put utf8NoBOM
. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs say that UTF8NoBOM
is already the default
https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.management/set-content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bergmeister!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is UTF8NoBOM in PSCore, but to make it work in Windows PowerShell too I think we have to fall back to .NET
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we might not need this to work in Windows PowerShell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to WriteAllLines .NET API to support Windows PowerShell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can we not drop WindowsPowerShell for building purposes only? Developers are not users and I added a pwsh
switch to the Azure DevOps task last year and it seems we are not using AppVeyor any more.
I was thinking of changing this line here to use use pwsh
instead lately btw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably want to move to pwsh
only for building, but I suspect currently we just pick a PowerShell and stick with it. So what @TylerLeonhardt's got is safest for now.
PR Summary
Fixes #1880
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
x
between the square brackets.Please mark anything not applicable to this PR
NA
.WIP:
to the beginning of the title and remove the prefix when the PR is ready