-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Change New-ModuleManifest encoding to UTF8NoBOM on non-Windows platforms #3940
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
Conversation
| this, | ||
| filePath, | ||
| EncodingConversion.Unicode, | ||
| new UTF8Encoding(false), // UTF-8, no BOM |
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.
Do we need to update the documentation for this change?
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.
@anmenaga says he opened a doc issue, can you link it here?
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 PR in RFC repo.
|
As it currently stands, this is a breaking change (appropriate label added). Based on our current POR, we're okay changing the default on non-Windows platforms, but this shouldn't be changed (yet) on Windows until we've had the breaking change conversation. /cc @JamesWTruher |
|
I understand that this currently breaks the |
|
Are there plans to add UTF8-NoBom to powershell supported Encodings? Changing the most important scenarios is good, but it would be great to address the problem with encodings broader and consistently to help shape the conversation about breaking changes. |
|
@vors UTF8noBOM is discussed in RFC 0020 DefaultFileEncoding. |
|
New-PSSessionConfigurationFile uses |
|
@vors: first, yes. We're adding that (and making it the default across the board on Linux). The hard part is what to do with Windows (change the defaults? leave a legacy enum value for the "traditional" disparate defaults?) Which is why I want to have the discussion. |
|
@joeyaiello good point; |
|
Is there an actual problem on Windows using UTF8NoBOM? If only English is used, it's equivalent to an ascii file and Windows tools handle that just fine and I expect that to be the majority of use. |
|
Restarted AppVeyor due to MyGet connectivity issue |
|
@anmenaga if you could sync with @JamesWTruher before merging this, that'd be awesome. Assuming the @PowerShell/powershell-committee votes to accept tomorrow, we want to move to UTF8NoBOM everywhere. |
Users can add non-English comments in the manifest. |
|
@iSazonov absolutely, but I believe the majority of usage is English. Even with non-ascii characters, my understanding is that notepad has heuristics to determine encoding to display it correctly. VSCode seems to support UTF8NoBom just fine. |
It seem works on last versions only not on Windows 7/8(?). |
|
@joeyaiello Are you saying this is blocked on a sign off from @JamesWTruher ? |
|
@joeyaiello @JamesWTruher Do we want to document the output format of Cmdlets? |
|
@TravisEz13 yes and yes. I think the uber-point is that @JamesWTruher is doing so much work here, that it might get changed anyway (and it might just muddle his extensive notes). Ultimately up to him, though. I just wanted to make sure everyone was on the same page. Regarding docs though: heck yes. We'll have a |
JamesWTruher
left a comment
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 is fine for now, I'll be changing all the cmdlets which provide for encoding as part of https://github.com/PowerShell/PowerShell-RFC/blob/master/1-Draft/RFC0020-DefaultFileEncoding.md. This doesn't muddy the waters for me.
| (Get-Content -Encoding Byte -Path $testModulePath -TotalCount $expected.Length) -join ',' | Should Be ($expected -join ',') | ||
| } | ||
|
|
||
| It "Verify module manifest encoding on Windows " -Skip:(-not $IsWindows) { |
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.
Could you just calculate the expected value in a beforeall then have one test for all platforms?
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.
Good idea; updated.
TravisEz13
left a comment
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.
Please respond to my comment on your code.
|
@anmenaga Also, as a PowerShell team member, you need to submit the PR to update the docs before I merge this. Please add a link to the PR here. Thanks. |
|
For encoding changes, you might want to delay documenting as we expect big changes in this. |
|
SteveL-MSFT
left a comment
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.
We should update https://github.com/PowerShell/PowerShell/blob/master/docs/FAQ.md with the recommended settings in VSCode for automatic removing of trailing whitespace for resx files
|
@anmenaga Per the conversation in this PR, we want to add encoding documentation to the CmdLet docs. |
|
@TravisEz13 Here is the doc PR: MicrosoftDocs/PowerShell-Docs#1405 |
|
My only issue here is that although this is a change that I agree with, I think that this is where I have concerns of this causing a disparate and inconsistent behaviour between all versions of PowerShell below v6 due to the fact that New-ModuleManifest is baked into the Microsoft.PowerShell.Core module. I personally would much prefer to see that this update to the cmdlet gets released in a manner that fully supports being used on downlevel systems where PowerShell v6 is unable to be installed. However the only real solutions that I can really see available to be able to make that to happen would be to
If this change could be easily replicated for downlevel versions of PowerShell I think that would be the best outcome all around. |
This addresses #3789
Currently New-ModuleManifest creates psd1 manifests in UTF-16 with BOM; this creates problems for Linux tools and Git diff'er.
This fix changes encoding of New-ModuleManifest to be UTF8 (no BOM).
Also mentioned New-ModuleManifest in "RFC 0020 DefaultFileEncoding".