-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Partial properties: misc compiler layer work #73527
Partial properties: misc compiler layer work #73527
Conversation
src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs
Outdated
Show resolved
Hide resolved
void verifySource(ModuleSymbol module) | ||
{ | ||
var prop = module.GlobalNamespace.GetMember<SourcePropertySymbol>("C.P"); | ||
Assert.True(prop.GetPublicSymbol().IsExtern); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider validating IsExtern
in other extern tests below (like Extern_02
)
verifyAccessor(prop.GetMethod!); | ||
verifyAccessor(prop.SetMethod!); | ||
|
||
if (isSource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We commonly do bool inSource = module is SourceModuleSymbol;
to avoid the extra parameter, thus allowing for symbolValidator : verify, sourceSymbolValidator: verify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll keep this in mind for future tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (iteration 7) modulo RegularNext in LangVer test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one small comment, otherwise LGTM.
_ => null | ||
}; | ||
|
||
if (implementationPart is not null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this existing check is actually correct for partial symbols that can't skip implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be concerned about a scenario like this: (SharpLab for analogous method scenario)
public partial class C {
void M0()
{
_ = Prop; // on hover do we see the doc comment?
}
/// <summary>Property comment</summary>
partial int Prop { get; } // no impl
}
Thankfully I think the !_isForSingleSymbol
logic on line 288 (at time of writing this comment) ensures that Quick Info does show a doc comment here. We could add a test.
|
||
/// <summary>Counterpart to <see cref="PartialMethod_Paramref_04"/>.</summary> | ||
[Fact] | ||
public void PartialIndexer_Paramref_04() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add soem doc tests when we're missing an implementation member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (iteration 10)
str.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries); | ||
|
||
public static void AssertLinesEqual(string expected, string actual) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context. Something about my testing workflow was causing trailing whitespace on lines of the test to be lost, but our asserts were sensitive to that whitespace. I was finding it really frustrating to dig up that whitespace (basically had to go into the debugger and poke at the actual documentation string) so went ahead and started using a whitespace-insensitive comparison (though still line-sensitive) for a few of the tests.
src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs
Outdated
Show resolved
Hide resolved
ee47efe
into
dotnet:features/partial-properties
Test plan: #73090
Related: dotnet/csharplang#8131