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

[CyrusNajmabadi]

Fixes #6718

Tagging @dotnet/roslyn-ide

[jasonmalinowski]

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.

[CyrusNajmabadi]

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').

[CyrusNajmabadi]

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

[jasonmalinowski]

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.

[jasonmalinowski]

👍

[brettfo]

I'm not on board with this (yet). You can't compare the prefix and completely ignore case without first checking that the next character is upper case (assuming a caml/pascal cased identifier) or an underscore (C-style). Presumably you'll match things like setValue/getValue, but what about a class that looks like this:

class VacationDestination
{
    public string GetawayName() { return "Mexico"; }
}

I definitely don't want to be offered this:

    public string awayName() { get { return "Mexico"; } }
    // or
    public string awayName() => "Mexico";

Granted, the case I mention above will currently fail the existing implementation, but there are other examples, too. E.g., what if I'm flagging my methods' return type with a character like so (g = Guid):

public Guid gEtymologyId() { return new Guid("..."); }

This is even worse than the above example:

public Guid ymologyId() => new Guid("...");

Ninja edit:
If we're going to be smart and detect these things, let's actually be smart about what we detect.

[jasonmalinowski]

After @brettfo's statements, I retract my 👍.

[CyrusNajmabadi]

@brettfo I'm confused. What you're asking for us precisely what the HasPrfix function does. It only matches if the character after the matched "get" is a capital letter...

[CyrusNajmabadi]

Both the GetawayName and gEtymologyId cases should be handled fine with the existing code. We'll offer to change GetawayName() to a property called "GetawayName" (i.e. just removing the parameters). The same will be true of the gEtymology case. In neither of these cases will we strip out the "Get" prefix, regardless of how it is cases.

[davkean]

Why would we suggest changing GetawayName() (method) -> GetawayName (property)?

[davkean]

Oh, because this is always available?

[CyrusNajmabadi]

Why would we suggest changing GetawayName() (method) -> GetawayName (property)?

Because that's the feature... :)

You have something which you realize you want to be a property, and so we offer to make it a property for you. i.e.

class Person
{
    public string Name() { }
}

"Whoops. I really want that to be a property. But it's a PITA to go update all my code that is referencing. Yaaay, Roslyn to the rescue!"

[jasonmalinowski]

But if you want to preserve the name, then this refactoring isn't worth nearly as much. So do we care? I totally get (and love) the Get/Set case since we'll rename all sorts of things...but in your case it's just a trivial syntax change a user can do without the tool already.

[brettfo]

After much internal debate I'm going to give my approval because the cases where this makes a name change the user doesn't expect is small, and even then it can be fixed by a follow-up rename.

[Pilchie]

It's not trivial to remove all the empty () at all the callsites.

[CyrusNajmabadi]

but in your case it's just a trivial syntax change a user can do without the tool already.

Really? I think it's super obnoxious to have to fix up X number of invocations all over your code. Similarly, you thn need to go fix up any and all things like interface implementations, overrides, etc. of your method.

I mean, i agree. At the end of the day, this is fairly marginal feature. I don't expect people will use it nearly as often as our others. But when they need it, i think it will speed things up. And especially if they're porting code this will be handy :)

BTW, you've just described a huge swath of resharper fixes. My hope is eventually we get to the point where we can have their level of "tons of simple, nice to have time savers". They don't have to be earth shattering. They just need to be ubiquitous, always helping me out. :)

[jasonmalinowski]

OK, I'm an idiot and forgot about the callsite () removal. Finding more coffee...

[CyrusNajmabadi]

Note: Changing the SetXXX calls is even more fun. We have to replace the invocation expression with an assignment expression. And we do that all correctly, even with things like nested invocations :-)

-- Cyrus

---

From: Jason Malinowskimailto:notifications@github.com
Sent: ‎11/‎16/‎2015 5:46 PM
To: dotnet/roslynmailto:roslyn@noreply.github.com
Cc: Cyrus Najmabadimailto:cyrusn@microsoft.com
Subject: Re: [roslyn] Allow the 'replace method with property' refactoring to work with get/set methods, not just Get/Set methods. (#6799)

OK, I'm an idiot and forgot about the callsite () removal. Finding more coffee...

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6799#issuecomment-157197478.