Skip to content

Allow the 'replace method with property' refactoring to work with get/set methods, not just Get/Set methods. #6799

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.

Sign up for GitHub

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

Merged
CyrusNajmabadi merged 1 commit into from
Nov 17, 2015
Merged

Allow the 'replace method with property' refactoring to work with get/set methods, not just Get/Set methods. #6799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...atures/CSharpTest/CodeActions/ReplaceMethodWithProperty/ReplaceMethodWithPropertyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,15 @@ public void TestWithPartialClasses()
Test(
@"partial class C { int [||]GetFoo() { } } partial class C { void SetFoo(int i) { } }",
@"partial class C { int Foo { get { } set { } } } partial class C { }",
index: 1);
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsReplaceMethodWithProperty)]
public void TestUpdateGetSetCaseInsensitive()
{
Test(
@"using System; class C { int [||]getFoo() { } void setFoo(int i) { } }",
@"using System; class C { int Foo { get { } set { } } }",
index: 1);
}
}
Expand Down
10 changes: 8 additions & 2 deletions ...re/Portable/ReplaceMethodWithProperty/ReplaceMethodWithPropertyCodeRefactoringProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,20 @@ private static bool HasGetPrefix(SyntaxToken identifier)

private static bool HasGetPrefix(string text)
{
return text.StartsWith(GetPrefix) && text.Length > GetPrefix.Length && !char.IsLower(text[GetPrefix.Length]);
return HasPrefix(text, GetPrefix);
}
private static bool HasPrefix(string text, string prefix)
{
return text.StartsWith(prefix, StringComparison.OrdinalIgnoreCase) && text.Length > prefix.Length && !char.IsLower(text[prefix.Length]);
Copy link
Member

@jasonmalinowski jasonmalinowski Nov 16, 2015

Choose a reason for hiding this comment

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

Should we be using CaseInsensitiveComparer here? Also, while you're here, you may want to move the text.Length > prefix.Length check first to speed things up.

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Nov 16, 2015

Choose a reason for hiding this comment

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

So, i'm a little unclear on when we should use each different comparer. I'm pinging @davkean about this since he was dealing with this with the Turkish I problem.

Hey @davkean. Could we write up a page somewhere that describes in what circumstances you'd want to use each specific type of comparer (including possibly 'never').

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Nov 16, 2015

Choose a reason for hiding this comment

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

Good call on the shortcircuiting though. Will def do that.

Copy link
Member

@jasonmalinowski jasonmalinowski Nov 16, 2015

Choose a reason for hiding this comment

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

VB, at least, has special rules for historical compiler bugs, which is why the compiler API exists. I believe in general the answer for VB at least is one should never use OrdinalIgnoreCase because the compiler itself is not using that. We violate this everywhere.

}

private IMethodSymbol FindSetMethod(IMethodSymbol getMethod)
{
var containingType = getMethod.ContainingType;
var setMethod = containingType.GetMembers("Set" + getMethod.Name.Substring(GetPrefix.Length))
var setMethodName = "Set" + getMethod.Name.Substring(GetPrefix.Length);
var setMethod = containingType.GetMembers()
.OfType<IMethodSymbol>()
.Where(m => setMethodName.Equals(m.Name, StringComparison.OrdinalIgnoreCase))
.Where(m => IsValidSetMethod(m, getMethod))
.FirstOrDefault();

Expand Down