Generate Constructor should not copy "protected" from abstract base class (#25238)#25866
Conversation
…te-public-constructor-for-concrete-class
…te-public-constructor-for-concrete-class
…te-public-constructor-for-concrete-class
|
|
|
|
||
| namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.GenerateDefaultConstructors | ||
| { | ||
| public class GenerateConstructorForAbstractClassTests : AbstractCSharpCodeActionTest |
There was a problem hiding this comment.
Thanks for the PR :)
we don't need a new test class for these tests. It should just go in the primary test class htat already exists for this feature. You can put it in another partial-part if you want to (though i would not recommend that personally).
There was a problem hiding this comment.
Thank you for Code Review 👍
Done.
| public async Task TestGenerateConstructorFromProtectedConstructor() | ||
| { | ||
| await TestInRegularAndScriptAsync( | ||
| @"abstract class C : [||]B |
There was a problem hiding this comment.
you don't have to change up all these test. But i found them odd to read as you had to read out of order. isntead of seeing hte base type, then the dependent type, you see the dependent type and then have to find the base type.
There was a problem hiding this comment.
Yeah, I agree. The tests were written in the same way as in GenerateDefaultConstructorsTests.cs.
| var classType = _state.ClassType; | ||
| var accessibility = baseConstructor.ContainingType.IsAbstractClass() ? | ||
| (classType.IsAbstractClass() ? baseConstructor.DeclaredAccessibility : Accessibility.Public) | ||
| : baseConstructor.DeclaredAccessibility; |
There was a problem hiding this comment.
IDE style would be:
var accessibility = baseConstructor.ContainingType.IsAbstractClass()
? classType.IsAbstractClass() ? baseConstructor.DeclaredAccessibility : Accessibility.Public
: baseConstructor.DeclaredAccessibility;There was a problem hiding this comment.
or:
var accessibility = baseConstructor.ContainingType.IsAbstractClass()
? classType.IsAbstractClass()
? baseConstructor.DeclaredAccessibility
: Accessibility.Public
: baseConstructor.DeclaredAccessibility;There was a problem hiding this comment.
interesting bit of logic... I would personally just prefer:
var accessibility = baseConstructor.ContainingType.IsAbstractClass() && !classType.IsAbstractClass()
? Accessibility.Public
: baseConstructor.DeclaredAccessibility;There was a problem hiding this comment.
Done like in Neme12's suggestion
…te-public-constructor-for-concrete-class
|
@jinujoseph 15.7? |
|
@angelina-dev Thanks much for the PR. |
|
@jinujoseph for 15.8 do I need approval? or sign off is all I need? |
|
no ask approvals needed |
|
@dotnet/roslyn-infrastructure jenkins issue D:\j\workspace\windows_relea---a23004dd\Binaries\Tools\dotnet\sdk\2.1.300-preview2-008324\NuGet.targets(114,5): error : Failed to retrieve information about 'Microsoft.VisualStudio.Text.UI.Wpf' from remote source https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_unit32_prtest/12806/console |
|
again, Jenkins issue. myget is down. |
|
retest windows_release_unit32_prtest please |
|
Awesome! I've been wanting this for awhile. |
fixes : #25238
There was no answer to my question in comments for the issue: "If a base class is not abstract, it looks like we should make the constructor of the derived class public. What do you think?". So the implementation did not affect this case and a constructor is still generated with the same access modifier as base constructor.