Add global:: to default usings
#8314
Conversation
jjonescz
force-pushed
the
7091-System-in-namespace
branch
from
dbd745a to
487d77a
Compare
jjonescz
force-pushed
the
7091-System-in-namespace
branch
from
487d77a to
7dc27bf
Compare
| context.CodeWriter | ||
| .Write("((") | ||
| .Write(typeof(Action).FullName) | ||
| .Write(")("); |
There was a problem hiding this comment.
💭 I think I might prefer the form on the left, and just change (( to ((global::
| ((global::System.Action)(() => { | ||
| #nullable restore | ||
| #line 1 "x:\dir\subdir\Test\TestComponent.cshtml" | ||
| global::System.Object __typeHelper = nameof(X.System.Y); |
There was a problem hiding this comment.
📝 I'm expecting this to be just object by the end of this change
There was a problem hiding this comment.
I don't plan to update everything to object (that's orthogonal to the issue), I just fixed cases where there was System.Object without global::, failing the new test cases. I could perhaps use global::System.Object everywhere but object seemed better.
There was a problem hiding this comment.
object is definitely better.
|
|
||
| codeDocument.SetDocumentIntermediateNode(document); | ||
|
|
||
| static bool TryRemoveGlobalPrefixFromDefaultUsing(in UsingReference usingReference, out UsingReference trimmed) |
There was a problem hiding this comment.
📝 It's difficult to review the impact of this change in isolation because baselines weren't updated with each change
There was a problem hiding this comment.
That's because this change caused failures - there was a new warning about duplicate usings in the new tests. I don't think anything changed in the old tests. But I could verify that.
| [InlineData("Project.Foo", "global::Project.Bar", false)] | ||
| [InlineData("Project.Bar.Foo", "Project", false)] | ||
| [InlineData("Bar.Foo", "Project", false)] | ||
| public void IsTypeInNamespace_WorksAsExpected(string typeName, string @namespace, bool expected) |
There was a problem hiding this comment.
💡 expected shouldn't be a parameter here. It should be a variable inside the test which is initialized with a switch expression, e.g.:
var expected = (typeName, @namespace) switch
{
("", "") => true,
("Foo", "Project") => true,
("Project.Foo", "Project") => true,
("Project.Foo", "global::Project") => true,
_ => false,
};
There was a problem hiding this comment.
Why would that be better? It would need duplicating the test data.
There was a problem hiding this comment.
Why would that be better?
The parameters to a test are part of the test identity. When expected outcomes are part of the parameter list, it is impossible to update code in a manner that changes the expected outcome of a test without deleting the entire test history. This improves both CI behavior and local iterations as the test is initially developed.
It would need duplicating the test data.
Yes, and for this type of situation it's completely acceptable. Duplication is not as much a maintenance problem when coupled (both copies are highly likely to appear on the same screen at the same time).
There was a problem hiding this comment.
That's interesting. I wouldn't think expected outcomes of unit tests change, that's what snapshot tests are for. Although I guess the line between the two is not that clear.
|
Alright, I can split this into multiple PRs. Marking as draft for now. |
3 participants
Fixes #7091.