Skip to content
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

Offer to add parameter to constructor with no existing parameters #33624

Merged
merged 3 commits into from
Feb 28, 2019

Conversation

chborl
Copy link
Contributor

@chborl chborl commented Feb 23, 2019

Fixes #33602

@chborl chborl requested a review from a team as a code owner February 23, 2019 00:30
IMethodSymbol constructor,
ImmutableArray<string> parametersName)
{
var constructorParams = constructor.Parameters;
return constructorParams.Length > 0
&& constructorParams.All(parameter => parameter.RefKind == RefKind.None)
if (constructorParams.Length <= 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the length of array can't be less than 0, == is sufficient.

@genlu
Copy link
Member

genlu commented Feb 26, 2019

        /// 1. Constructor with empty parameter list.

This comment needs update as well


Refers to: src/Features/Core/Portable/AddConstructorParametersFromMembers/AddConstructorParametersFromMembersCodeRefactoringProvider.State.cs:81 in 817e29d. [](commit_id = 817e29d, deletion_comment = False)

&& constructorParams.All(parameter => parameter.RefKind == RefKind.None)
if (constructorParams.Length <= 0)
{
return !constructor.IsImplicitlyDeclared;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the behavior if we only have an implicit default ctor? Refactoring will not be available?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i would prefer it's avialable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this: #33624 (comment)


[WorkItem(33602, "https://github.com/dotnet/roslyn/issues/33602")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddConstructorParametersFromMembers)]
public async Task TestConstructorWithNoParameters()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also have a test with only implicit default ctor?

Copy link
Contributor Author

@chborl chborl Feb 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we have this test:

class C
{
    [|int i;|]
    int Hello { get; set; }
}

{
[|int i;|]
int Hello { get; set; }
}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably shoudl offer to create an explicit constructor here since hte implicit one def exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "generate constructor" refactoring is offered here, and it will generate the constructor with the selected field as a parameter.

The "add parameters to constructor" is not offered, but I think that is expected.

@chborl chborl merged commit c5a6a85 into dotnet:master Feb 28, 2019
333fred added a commit to 333fred/roslyn that referenced this pull request Mar 4, 2019
* dotnet/master: (903 commits)
  Add UseEnhancedColors to FeatureOnOffOptionsProvider (dotnet#33802)
  Update TypeStyle.cs
  Text on a InterpolatedVerbatimStringStartToken token (dotnet#33747)
  added ability to clear all diagnostics reproted from a IDiagnosticUpd… (dotnet#33805)
  Use a set instead of map to define processed redundant nodes.
  EnC: Remove dependency on solution from AnalyzeDocumentAsync (dotnet#33796)
  Add workitem links to new redundant assignment tests.
  Move leading trivia with node when removing unused values.
  Remove OOP related feature options. (dotnet#31644)
  Merge pull request dotnet#33631 from CyrusNajmabadi/wrapPriority
  Use the correct iteration count in IterationDataAttribute
  Handle interface members in NullableWalker.AsMemberOfType (dotnet#33764)
  Cleanly work around microsoft/nodejstools#2138
  Implement full spec changes for Index/Range (dotnet#33679)
  Further reduce expectations on deep fluent calls
  Lower expectation for deep fluent call
  Take screenshot after writing logs
  Avoid throwing exception in static constructor
  Offer to add parameter to constructor with no existing parameters (dotnet#33624)
  Add regression test for nullable crash (dotnet#33735)
  ...
@chborl chborl deleted the addparamtonoparamctor branch March 6, 2019 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants