Remove xmlns from props/targets and UTs#7169
Conversation
|
FYI @Nirmal4G since you were interested in this |
|
Also, if it's not too much trouble, can you separate your changes into specific commits. Like separate the formatting and Totally optional but it's just good practice. |
Forgind
left a comment
There was a problem hiding this comment.
I'm not going to pretend I legitimately reviewed this, but the part I did review looked good! xmlns and ToolsVersion confused me before I learned that I can pretty much ignore them.
There was a problem hiding this comment.
You can also remove the ToolsVersion, right?
There was a problem hiding this comment.
Yup, although I figured this PR was large enough as it was, so probably will follow up with another PR.
The formatting was automatic by vscoed and unintentional :(. But figured I'd leave them in place. I should look into turning that off by default... |
|
No problem. It's good that you format the files that is touched but bad that every change is in a single commit, that's all! |
Nirmal4G
left a comment
There was a problem hiding this comment.
Took a long time but it's looking good! 👌
There was a problem hiding this comment.
💡 Make sure to verify there is still adequate coverage for cases where xmlns is included, since both approaches are supported
There was a problem hiding this comment.
Yup, I left it in a bunch of UTs, mostly ones which dealt with evaluation. It probably could be cleaned from even more UTs (many use xmlns="msbuildnamespace" instead of xmlns="http://schemas.microsoft.com/developer/msbuild/2003">), but whatever this change is large enough as-is.
Comparing repo-wide searches of "xmlns=" (which admittedly covers other namespaces eg in loc xml files):
Before: 2641 results in 377 files
After: 1693 results in 277 files
There was a problem hiding this comment.
This also had me worried but by inspection it looks ok (this concern is why I split out the "remove from unit tests" part of the change).
src/Build.OM.UnitTests/Construction/ProjectItemElement_Tests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
is dogfood relevant here? it's deprecated so...meh
There was a problem hiding this comment.
Do deprecated unit tests even run? Like ever? Should we just delete this?
There was a problem hiding this comment.
they don't at the moment but I hate to delete them because we should turn them back on (but maybe we can just delete the code first?
There was a problem hiding this comment.
More than happy to delete deprecated UTs if that's the decision. But for another PR, as this one is big enough :)
benvillalobos
left a comment
There was a problem hiding this comment.
Checked everything but the .cs files and it looks fine, minus my 1 question on compat. If we won't have any compat issues, I approve.
There was a problem hiding this comment.
This won't break compatibility with some super old MSBuild that's still in support will it?
There was a problem hiding this comment.
As with #7165, the assertion is that these targets are never used with a super-old MSBuild.
I left it in a bunch of UTs, mostly ones which dealt with evaluation. It probably could be cleaned from even more UTs (many use `xmlns="msbuildnamespace"` instead of `xmlns="http://schemas.microsoft.com/developer/msbuild/2003">`), but whatever this change is large enough as-is. Comparing repo-wide searches of "xmlns=" (which admittedly covers other namespaces eg in loc xml files): Before: 2641 results in 377 files After: 1693 results in 277 files
348b71f to
af7c186
Compare
|
I just pushed a change that a) splits into two commits, one for "MSBuild logic in/shipped from this repo" and one for tests, and b) drops the changes in |
xmlns hasn't been requires in project files for a while now (v15?), so this change removes it from all props/targets as well as all UTs (minus the ones explicitly testing the xmlns stuff).