kswoll / ReactiveUI.Fody

C# Fody extension to generate RaisePropertyChange notifications for properties and ObservableAsPropertyHelper properties.
MIT License
154 stars 31 forks source link

Add a roslyn CodeFix for converting properties to Reactive #27

Open bradphelan opened 7 years ago

bradphelan commented 7 years ago

A roslyn code fix that can convert

class TypeName
{   
     int _Foo = 10;
     int Foo { get { return _Foo; } {set _Foo = value;}}
}

to

 class TypeName
 {
    [Reactive]
    int Foo { get; set; }
 }

Useful for converting large code bases.

It checks if the [Reactive] attribute is applied already but will convert any property. Activating solution wide is probably dangerous as it will convert any and all properties to [Reactive].

I haven't integrated the nuget packages yet so the analyser/codefix is shipped automatically.

kswoll commented 7 years ago

This is a great idea! When converting an existing code base that is using rxui, The idiomatic implementation would be:

class TypeName
{   
    int _Foo = 10;
    int Foo 
    { 
        get { return _Foo; } 
        set { this.RaiseAndSetIfChanged(ref _Foo, value); }
    }
}

I wonder if we could have a fix that is more discriminating and checks for this idiom for the purposes of a solution-wide code-fix. Anyway, will take a look at this in the next day and get it merged soon.

Thanks for contributing!

bradphelan commented 7 years ago

I could try to make it more discriminating. It was my first roslyn code fix and a steep learning curve but it might be easier on second look. It is certainly a bit dangerous in its current form.

On Tue, 22 Nov 2016, 20:24 Kirk Woll notifications@github.com wrote:

This is a great idea! When converting an existing code base that is using rxui, The idiomatic implementation would be:

class TypeName { int _Foo = 10; int Foo { get { return _Foo; } set { this.RaiseAndSetIfChanged(ref _Foo, value); } } }

I wonder if we could have a fix that is more discriminating and checks for this idiom for the purposes of a solution-wide code-fix. Anyway, will take a look at this in the next day and get it merged soon.

Thanks for contributing!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kswoll/ReactiveUI.Fody/pull/27#issuecomment-262339187, or mute the thread https://github.com/notifications/unsubscribe-auth/AABE8tS84OCOUClnxCWz48mHwobh3M5cks5rA0FjgaJpZM4K5Mqh .

kswoll commented 7 years ago

Hey, sorry, was on vacation and didn't have a chance to review this PR. I'm back now and will take a look tonight or tomorrow.

bradphelan commented 7 years ago

I'm going to take a look at tightening the analyser.

kswoll commented 7 years ago

Thanks, and yeah, I've been trying to do some research on how to get this integrated into the existing nuget package.

bradphelan commented 7 years ago

Looking forward to having it integrated. I have load of legacy code to fix. I feel I cheated by dropping back to string comparison but I just don't know the roslyn model well enough.

On Wed, 30 Nov 2016, 19:50 Kirk Woll notifications@github.com wrote:

Thanks, and yeah, I've been trying to do some research https://msdn.microsoft.com/en-us/magazine/mt573715.aspx on how to get this integrated into the existing nuget package.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kswoll/ReactiveUI.Fody/pull/27#issuecomment-263959324, or mute the thread https://github.com/notifications/unsubscribe-auth/AABE8nSdrvNvaOAisJbpc_HA1qrij4qkks5rDcWFgaJpZM4K5Mqh .

IQuixote commented 7 years ago

It would be amazing if this was integrated and worked! I tried playing around with it a little and got it working for separate instances, and you're right - the learning curve is steep.

kswoll commented 7 years ago

@IQuixote the thing is I'd love for it to "just work" and appear when you install this package. I've looked into it some, but it kind of overwhelmed me. But if we can get a PR, or at least some guidance, on how to get it into a state where nugetting ReactiveUI.Fody also installs this analyzer, it would be merged straightwaway.

escamoteur commented 7 years ago

But how would you control if it converts all properties or not? Or only for one class? If for all this will bow up the code