Fix #26358: AbstractDocumentationCommentCommandHandler has hardcoded Windows line ending#27626
Merged
jasonmalinowski merged 2 commits intodotnet:masterfrom Jun 11, 2018
Merged
Conversation
…coded Windows line ending I used `\n` and `\r\n` in string instead `@""` because as far as I understand on Windows/CI code is checked out with `git config core.autocrlf` which means it modifies line endings...
a63d7a4 to
5508d1c
Compare
Contributor
Author
|
Hmm, looks like unit tests infrastructure just replace |
…t infrastructure assumption that all code is `\r\n`
Contributor
Author
|
@dotnet-bot retest windows_debug_vs-integration_prtest please |
Contributor
Author
|
@dotnet-bot retest windows_debug_vs-integration_prtest please |
jasonmalinowski
approved these changes
Jun 8, 2018
| private string GetNewLine(SourceText text) | ||
| { | ||
| // return editorOptionsFactoryService.GetEditorOptions(text).GetNewLineCharacter(); | ||
| return "\r\n"; |
jinujoseph
approved these changes
Jun 9, 2018
Contributor
|
Approved to merge for 15.8.Preview4 |
Member
|
Thanks @DavidKarlas for the fix! I worry there are plenty more of these hiding... |
Contributor
Author
|
@jasonmalinowski first step in finding them would be removing hacks like https://github.com/dotnet/roslyn/blob/master/src/EditorFeatures/TestUtilities/Extensions/XElementExtensions.cs#L12 and not using |
Member
|
Well, at the time NormalizedValue wasn't a hack: VB XML literals follow the XML spec which state that they're linefeed only even on Windows machines -- hence that converts to what we expected at the time. That said, I'd be 100% for using the platform's line endings in tests, and then just enabling our tests on Linux/Mac runs. Thoughts? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I used
\nand\r\nin string instead@""because as far as I understand on Windows/CI code is checked out withgit config core.autocrlfwhich means it modifies line endings...Customer scenario
If customer uses
\nfor line endings and has appropriate formatting setting set, every time documentation after///is inserted it inserted\r\nwrongly.Bugs this fixes
#26358
Workarounds, if any
None.
Risk
Fix is pretty small, so I would say it's unlikely to cause regressions.
Performance impact
I don't expect changes
Is this a regression from a previous update?
No.
Root cause analysis
There was commented code that would support thiis, so I would assume reason was just not caring about
\nscenario. Unit test is added.How was the bug found?
While integrating this command with VSfM.
Test documentation updated?
Unit tests should be sufficient.