X Tutup
Skip to content

Replace CR and new line with a 0x23CE character#10616

Merged
adityapatwardhan merged 8 commits intoPowerShell:masterfrom
TravisEz13:fix_linux_syslogs
Oct 7, 2019
Merged

Replace CR and new line with a 0x23CE character#10616
adityapatwardhan merged 8 commits intoPowerShell:masterfrom
TravisEz13:fix_linux_syslogs

Conversation

@TravisEz13
Copy link
Member

@TravisEz13 TravisEz13 commented Sep 24, 2019 •
edited
Loading

PR Summary

Replace CR and new line with a 0x23CE character

PR Context

Trying to document how to use logging with Azure Log Analytics and RSyslog converting these to #012 and #013

PR Checklist

@TravisEz13 TravisEz13 added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Sep 24, 2019
@TravisEz13 TravisEz13 added this to the 7.0.0-preview.5 milestone Sep 24, 2019
@TravisEz13 TravisEz13 changed the title Replace CR and new line with a 0x23CE character WIP: Replace CR and new line with a 0x23CE character Sep 24, 2019
daxian-dbw
daxian-dbw reviewed Sep 25, 2019
View reviewed changes
src/System.Management.Automation/engine/runtime/CompiledScriptBlock.cs Outdated
textToLog = textToLog.Replace('\u0000', '\u2400');
#if UNIX
if (Platform.IsLinux)
{
Copy link
Member

@daxian-dbw daxian-dbw Sep 25, 2019

Choose a reason for hiding this comment

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

This check is not needed.

Copy link
Member Author

@TravisEz13 TravisEz13 Sep 25, 2019

Choose a reason for hiding this comment

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

Why? This is only an issues for SysLog, not OsLog on macOS

Copy link
Member

@daxian-dbw daxian-dbw Sep 25, 2019

Choose a reason for hiding this comment

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

Got it. Closed

Copy link
Collaborator

@iSazonov iSazonov Sep 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In the case it would be good to have the protection comment in the code.

Copy link
Member Author

@TravisEz13 TravisEz13 Oct 3, 2019

Choose a reason for hiding this comment

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

I can repeat the comment if you want

Copy link
Member Author

@TravisEz13 TravisEz13 Oct 3, 2019

Choose a reason for hiding this comment

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

It is explained on line 1584

@daxian-dbw
Copy link
Member

daxian-dbw commented Sep 25, 2019

Talked with @TravisEz13 about the concern of creating 3 large strings for Linux when the script itself is large.
@TravisEz13 will update the code to address that concern.

adityapatwardhan
adityapatwardhan approved these changes Sep 25, 2019
View reviewed changes
daxian-dbw
daxian-dbw reviewed Sep 25, 2019
View reviewed changes
@TravisEz13 TravisEz13 changed the title WIP: Replace CR and new line with a 0x23CE character Replace CR and new line with a 0x23CE character Oct 3, 2019
iSazonov
iSazonov reviewed Oct 4, 2019
View reviewed changes
test/powershell/Host/Logging.Tests.ps1
[string] $powershell = Join-Path -Path $PSHome -ChildPath 'pwsh'
$scriptBlockCreatedRegExTemplate = @"
Creating Scriptblock text \(1 of 1\):#012{0}(`u{23CE}|\?|#012)*ScriptBlock ID: [0-9a-z\-]*#012Path:.*
Creating Scriptblock text \(1 of 1\):#012{0}(⏎|#012)*ScriptBlock ID: [0-9a-z\-]*#012Path:.*
Copy link
Collaborator

@iSazonov iSazonov Oct 4, 2019

Choose a reason for hiding this comment

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

Why do we need this? `u{23CE} looks more clear for me.

Copy link
Member Author

@TravisEz13 TravisEz13 Oct 4, 2019

Choose a reason for hiding this comment

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

The tests fail if we use the escape sequence.

Copy link
Collaborator

@iSazonov iSazonov Oct 4, 2019

Choose a reason for hiding this comment

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

I tried to copy and paster the code in console and lost the char. Also the tests become unreadable. I wonder why doesn't the Unicode syntax sugar work?

Copy link
Member Author

@TravisEz13 TravisEz13 Oct 4, 2019

Choose a reason for hiding this comment

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

@iSazonov that's going to vary by the font you use. I worked with @rjmholt to come up with this solution. The previous solution that was working, was actually just looking for a ? which is less specific.

Copy link
Collaborator

@iSazonov iSazonov Oct 4, 2019

Choose a reason for hiding this comment

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

I remember that we had Unicode chars in files and it was a headache - editors silently break them. After that we made a conclusion to keep all files in ASCII.

@adityapatwardhan adityapatwardhan merged commit bf91246 into PowerShell:master Oct 7, 2019
@TravisEz13 TravisEz13 deleted the fix_linux_syslogs branch October 7, 2019 18:44
@ghost
Copy link

ghost commented Oct 23, 2019

🎉v7.0.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw left review comments

@iSazonov iSazonov iSazonov left review comments

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

+1 more reviewer

@PoshChan PoshChan PoshChan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

7.0.0-preview.5

Development

Successfully merging this pull request may close these issues.

5 participants

@TravisEz13 @daxian-dbw @adityapatwardhan @iSazonov @PoshChan
X Tutup