Support preserving formatting when opening a project #1036

dsplaisted · 2016-09-16T06:25:46Z

Fixes #707

Adds the following overloads to ProjectRootElement:

public static ProjectRootElement Create(XmlReader xmlReader, ProjectCollection projectCollection, bool preserveFormatting)
public static ProjectRootElement Open(string path, ProjectCollection projectCollection, bool preserveFormatting)

Fixes dotnet#707

…be preserved when creating project from XmlReader

… preserve formatting is enabled

… likely to fix this

rainersigwald · 2016-09-16T15:02:19Z

We need to make sure this doesn't conflict with #1004, where @maddin2016 is playing in similar areas.

rainersigwald · 2016-09-16T15:11:02Z

src/XMakeBuildEngine/Evaluation/ProjectRootElementCache.cs

@@ -227,6 +227,8 @@ internal ProjectRootElement Get(string projectFile, OpenProjectRootElement openP
                            // use: it checks the file content as well as the timestamp. That's better than completely disabling
                            // the cache as we get test coverage of the rest of the cache code.
                            XmlDocument document = new XmlDocument();
+                            //  TODO: does PreserveWhitespace need to be an option here?
+                            //document.PreserveWhitespace = true;


Seems like it'd need to match what gets inserted into the cache, which would depend on how it was loaded, right?

I've updated this so that it won't use the cached version if it doesn't match the preserveFormatting setting requested.

rainersigwald · 2016-09-16T15:13:36Z

src/XMakeBuildEngine/Construction/ProjectElementContainer.cs

+                {
+                    var newWhitespaceNode = XmlDocument.CreateWhitespace(reference.XmlElement.PreviousSibling.Value);
+                    XmlElement.InsertAfter(newWhitespaceNode, reference.XmlElement);
+                }


It's unfortunate that we have to do this manually. But this seems like a reasonable strategy (and good comment).

But can it be abstracted and reused?

How do you mean abstracted and reused? ProjectElementContainer seems like it is already the generic place to put this logic.

This and below have very similar logic to find, clone, and insert whitespace. Though I guess it's only twice, so the generalization may not be that general.

Yeah, I didn't see an elegant way to unify the different cases that exist within ProjectElementContainer.

rainersigwald · 2016-09-16T15:17:27Z

    /// DO NOT BREAK THIS VERY USEFUL SAFETY CONTRACT.

Does this still hold?

Refers to: src/XMakeBuildEngine/Construction/ProjectElementContainer.cs:305 in 1a2e1da. [](commit_id = 1a2e1da, deletion_comment = False)

rainersigwald · 2016-09-16T15:19:15Z

src/XMakeBuildEngine/Construction/ProjectElementContainer.cs

+                else if (XmlElement.PreviousSibling != null &&
+                         XmlElement.PreviousSibling.NodeType == XmlNodeType.Whitespace)
+                {
+                    //  This container didn't have any whitespace in in.  This probably means it didn't have separate open


rainersigwald · 2016-09-16T15:20:12Z

src/XMakeBuildEngine/Construction/ProjectElementContainer.cs

+                //  So try to match the surrounding formatting and add one indentation level
+                if (XmlElement.FirstChild.NodeType == XmlNodeType.Whitespace)
+                {
+                    //  This container had a whitespace node, which should generally be a newline and the indent


how true is "generally"? Should we bother to validate this assumption?

I thought about this and it seemed OK to leave it as is. If there's spacing between the elements but not a newline, then we'll still be matching the surrounding formatting, even though the formatting seems weird.

Ok, I buy it. Can you put that in the comment?

rainersigwald · 2016-09-16T15:20:48Z

src/XMakeBuildEngine/Construction/ProjectElementContainer.cs

+                    //  and close tags.  So add a whitespace node before the new child with additional indentation over the
+                    //  container's indentation, and add a whitespace node with the same level of indentation as the container
+                    //  after the new child so the closing tag will be indented properly.
+                    string whitespace = XmlElement.PreviousSibling.Value;


rename to parentWhitespace?

rainersigwald · 2016-09-16T15:25:05Z

src/XMakeBuildEngine/UnitTestsPublicOM/Construction/ProjectFormatting_Tests.cs

+  <ItemGroup>
+    <ProjectReference Include=`..\CLREXE\CLREXE.vcxproj`>
+      <!-- Comment -->
+      <Project>{3699f81b-2d03-46c5-abd7-e88a4c946f28}</Project>


I'm not sure I prefer this behavior. It can be useful to have both

// Long comments explaining code;

and

code; // clarified

The behavior today is that comments are put on separate lines. If you specify the option to preserve formatting, then the location of the comment will be preserved. This test covers both behaviors, which is why you see the reformatted version.

Ohhh. Gotcha.

rainersigwald · 2016-09-16T15:30:12Z

src/XMakeBuildEngine/UnitTestsPublicOM/Construction/ProjectFormatting_Tests.cs

+            project.SaveLogicalProject(writer);
+
+            string actual = writer.ToString();
+            string expected = @"<?xml version=""1.0"" encoding=""utf-16""?>" +


attn: @AndyGerlicher

rainersigwald · 2016-09-16T15:30:31Z

src/XMakeBuildEngine/UnitTestsPublicOM/Construction/ProjectFormatting_Tests.cs

+            VerifyAssertLineByLine(expected, actual);
+        }
+
+        [Fact(Skip = "https://github.com/Microsoft/msbuild/issues/362")]


So the preserve-whitespace flag doesn't fix this? Boo :(

Apparently not... 😞

dsplaisted · 2016-09-16T18:55:43Z

@rainersigwald

    /// DO NOT BREAK THIS VERY USEFUL SAFETY CONTRACT.
Does this still hold?

Refers to: src/XMakeBuildEngine/Construction/ProjectElementContainer.cs:305 in 1a2e1da. [](commit_id = 1a2e1da, deletion_comment = False)

Yes. The comment is referring to the NextSibling and PreviousSibling properties of ProjectElement, which don't include the whitespace nodes.

rainersigwald · 2016-09-16T19:56:18Z

Ah, now I see that I evidently didn't hit "comment" on my most important comment. Sorry.

My instinct is to say that this shouldn't be an option--that is, we should always preserve whitespace. But you've plumbed through an option. Do you think that should be exposed to the user?

dsplaisted · 2016-09-16T20:47:38Z

I went with an opt-in option to avoid a compat risk (as suggested by Dave). How much of a compat risk would there be to always preserving formatting?

Also note that there's some risk that there are still cases where preserving the formatting will result in ugly formatting when the project is modified, as would be the case without the changes on ProjectElementContainer to add or remove whitespace when adding or removing child elements.

AndyGerlicher

Looks good!

davkean · 2016-09-27T17:36:59Z

@dsplaisted Where did we land with this? Did we sync with CPS to call this overload?

dsplaisted · 2016-09-27T17:47:38Z

@davkean No, AFAIK CPS isn't consuming this yet. I'm not sure if this has made it into a drop of MSBuild that they are using. @AndyGerlicher, can you comment?

srivatsn · 2016-11-17T19:24:34Z

@lifengl have we reacted to this in CPS?

dsplaisted and others added 11 commits September 15, 2016 23:17

Allow whitespace to be preserved when opening a project

113a947

Fixes dotnet#707

Add more test coverage for project formatting

c2e5b68

Rename preserveWhitespace to preserveFormatting, allow formatting to …

f8f8b3b

…be preserved when creating project from XmlReader

Move project formatting tests to a separate file

77892ec

Add test for issue dotnet#362

ccbc18e

Match surrounding indentation when adding a new item to a project and…

781c6ef

… preserve formatting is enabled

Add tests for formatting when adding item to empty or nonexistent group

2b52de3

Format element added to empty container

38cd89c

Support preserving formatting when removing elements

2bcaa27

Add tests for removing items and adding metadata

84e8312

Remove test for single quotes changing to double quotes as we are not…

1a2e1da

… likely to fix this

dnfclas added the cla-already-signed label Sep 16, 2016

Sarabeth-Jaffe-Microsoft added the Needs Review label Sep 16, 2016

rainersigwald reviewed Sep 16, 2016

View reviewed changes

Apply code review feedback

e0141a0

dsplaisted force-pushed the AllowPreserveWhitespace branch from 2269a7a to e0141a0 Compare September 16, 2016 20:43

AndyGerlicher approved these changes Sep 16, 2016

View reviewed changes

rainersigwald approved these changes Sep 16, 2016

View reviewed changes

dsplaisted merged commit 2db4d4b into dotnet:master Sep 16, 2016

Sarabeth-Jaffe-Microsoft removed the Needs Review label Sep 16, 2016

davkean mentioned this pull request Sep 27, 2016

Stop stripping whitespace dotnet/project-system#512

Closed

4 tasks

srivatsn mentioned this pull request Nov 17, 2016

Fix 4521: Add dotnet-add-p2p dotnet/cli#4758

Merged

rainersigwald mentioned this pull request Aug 1, 2017

Microsoft.Build.Evaluation.Project.Save() clears empty lines from a csproj #2376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support preserving formatting when opening a project #1036

Support preserving formatting when opening a project #1036

dsplaisted commented Sep 16, 2016

rainersigwald commented Sep 16, 2016

rainersigwald Sep 16, 2016

dsplaisted Sep 16, 2016

rainersigwald Sep 16, 2016

rainersigwald Sep 16, 2016

dsplaisted Sep 16, 2016

rainersigwald Sep 16, 2016

dsplaisted Sep 16, 2016

rainersigwald commented Sep 16, 2016

rainersigwald Sep 16, 2016

dsplaisted Sep 16, 2016

rainersigwald Sep 16, 2016

dsplaisted Sep 16, 2016

rainersigwald Sep 16, 2016

dsplaisted Sep 16, 2016

rainersigwald Sep 16, 2016

dsplaisted Sep 16, 2016

rainersigwald Sep 16, 2016

dsplaisted Sep 16, 2016

rainersigwald Sep 16, 2016

rainersigwald Sep 16, 2016

rainersigwald Sep 16, 2016

dsplaisted Sep 16, 2016

dsplaisted commented Sep 16, 2016

rainersigwald commented Sep 16, 2016

dsplaisted commented Sep 16, 2016

AndyGerlicher left a comment

davkean commented Sep 27, 2016

dsplaisted commented Sep 27, 2016

srivatsn commented Nov 17, 2016

Support preserving formatting when opening a project #1036

Support preserving formatting when opening a project #1036

Conversation

dsplaisted commented Sep 16, 2016

rainersigwald commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted commented Sep 16, 2016

rainersigwald commented Sep 16, 2016

dsplaisted commented Sep 16, 2016

AndyGerlicher left a comment

Choose a reason for hiding this comment

davkean commented Sep 27, 2016

dsplaisted commented Sep 27, 2016

srivatsn commented Nov 17, 2016