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

Replace property with method #11549

Merged
merged 41 commits into from
May 28, 2016

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented May 25, 2016

This PR adds a new refactoring where you can take an property and convert it to Get/Set methods.

@CyrusNajmabadi CyrusNajmabadi added PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. and removed cla-already-signed labels May 25, 2016
@CyrusNajmabadi CyrusNajmabadi removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 27, 2016
@CyrusNajmabadi
Copy link
Member Author

tagging @dotnet/roslyn-ide This is ready for review. Thanks!

@CyrusNajmabadi
Copy link
Member Author

retest vsi please

}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsReplacePropertyWithMethods)]
public async Task TestAutoProperty5()
Copy link
Member

Choose a reason for hiding this comment

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

Expression bodied properties, and properties with different access levels for their get and set methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are tests for hte former. But i will add tests for the latter. thanks!

// the case for C# where we have mangled names for the backing field and need something
// actually usable in code.
var uniqueName = NameGenerator.GenerateUniqueName(
property.Name.ToLowerInvariant(),
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't want to lowercase everything - just the first character?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch!

@Pilchie
Copy link
Member

Pilchie commented May 27, 2016

Looks pretty good to me with fixes for the above comments.

@CyrusNajmabadi
Copy link
Member Author

test vsi please

@CyrusNajmabadi
Copy link
Member Author

retest windows_vsi_p1_open_prtest please

@CyrusNajmabadi
Copy link
Member Author

retest windows_vsi_p2_open_prtest please

@CyrusNajmabadi CyrusNajmabadi merged commit b532942 into dotnet:future May 28, 2016
@CyrusNajmabadi CyrusNajmabadi deleted the replacePropertyWithMethod branch May 28, 2016 19:26
@CyrusNajmabadi CyrusNajmabadi restored the replacePropertyWithMethod branch May 30, 2016 21:04
@CyrusNajmabadi CyrusNajmabadi deleted the replacePropertyWithMethod branch January 25, 2020 00:39
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.

3 participants