-
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
Handle block comments with text on the same line as <# #> #2848
Handle block comments with text on the same line as <# #> #2848
Conversation
src/features/PowerShellNotebooks.ts
Outdated
} | ||
} | ||
} | ||
|
||
await vscode.workspace.fs.writeFile(targetResource, new TextEncoder().encode(retArr.join("\n"))); | ||
await vscode.workspace.fs.writeFile(targetResource, new TextEncoder().encode(retArr.join(EOL))); |
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 will replace newlines in a script when it changes operating systems. For example, if we have a script checked in to git and you edit it, then I edit it, I'll change all the newlines in it. Harder to implement an alternative, but something we should aim not to do -- I wouldn't want my text editor silently reencoding line endings as a side-effect of an unrelated operation. I think worth documenting and/or opening a new issue for
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.
So to solve this, I included metadata at the document level that says which line ending that file uses
@@ -78,7 +77,7 @@ function CreateCell(cellKind: vscode.CellKind, source: string[], metadata: IPowe | |||
cellKind, | |||
language: cellKind === vscode.CellKind.Markdown ? "markdown" : "powershell", | |||
outputs: [], | |||
source: source.join(EOL), | |||
source: source.join("\n"), |
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 only used in the rendering of notebooks so \n
is totally fine.
@rjmholt I believe this is good to go. |
PR Summary
Fixes #2845
This will better support block comments with stuff on the line:
and also make sure that saving doesn't mess up the style it previously used.
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