Skip to content

Fixed formatting on script blocks#18806

Merged
armanio123 merged 2 commits into
microsoft:masterfrom
armanio123:FixScriptBlockFormatting
Sep 28, 2017
Merged

Fixed formatting on script blocks#18806
armanio123 merged 2 commits into
microsoft:masterfrom
armanio123:FixScriptBlockFormatting

Conversation

@armanio123
Copy link
Copy Markdown
Contributor

Fixed formatting on script blocks, added regression tests, fixed minor bugs.

Comment thread src/services/formatting/formatting.ts Outdated
// if child is a list item - try to get its indentation, only if parent is within the original range.
let childIndentationAmount = Constants.Unknown;
if (isListItem) {
if (isListItem && parent.pos >= originalRange.pos && parent.end <= originalRange.end) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We've got a rangeContainsRange helper function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@armanio123 armanio123 merged commit a39110a into microsoft:master Sep 28, 2017
format.selection("begin4", "end4");
format.selection("begin5", "end5");

verify.currentFileContentIs("/* BEGIN EXTERNAL SOURCE */\n\n var a = 1;\n alert(\"/********/\");\n\n/* END EXTERNAL SOURCE */\n\n/* BEGIN EXTERNAL SOURCE */\n\n var b = 1;\n\n var c = \"/********/\";\n var d = 1;\n\n var e = \"/********/\";\n var f = 1;\n\n/* END EXTERNAL SOURCE */");
Copy link
Copy Markdown
Member

@billti billti Sep 28, 2017

Choose a reason for hiding this comment

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

Ooof. That's pretty ugly to have one long line with all the newline literals. It's hard to see what is going on. Using a multi-line string is dangerous as Git may checkout with different line endings, but could at least use line continuations to continue the string, e.g. something like:

verify.currentFileContentIs("\
/* BEGIN EXTERNAL SOURCE */\n\n\
                        var a = 1;\n\
                        alert(\"/********/\");\n\n\
/* END EXTERNAL SOURCE */\n\n\
/* BEGIN EXTERNAL SOURCE */\n\n\
            var b = 1;\n\n\
            var c = \"/********/\";\n\
            var d = 1;\n\n\
            var e = \"/********/\";\n\
            var f = 1;\n\n\
/* END EXTERNAL SOURCE */");

Copy link
Copy Markdown
Member

@weswigham weswigham Sep 28, 2017

Choose a reason for hiding this comment

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

git config core.autocrlf false will cause git to use whatever is actually in the repo, all the time. input or false should probably be required for our repo, since we do actually have some tests which are newline-sensitive (though most get around newline coercion by having mixed newlines...), and newline coercion is unneeded with modern editors and tooling. So a backtick-literal here would be super nice.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants