Skip to content

Add a roslyn CodeFix for converting properties to Reactive#27

Open
bradphelan wants to merge 5 commits intokswoll:masterfrom
bradphelan:feature/roslyn.codefix
Open

Add a roslyn CodeFix for converting properties to Reactive#27
bradphelan wants to merge 5 commits intokswoll:masterfrom
bradphelan:feature/roslyn.codefix

Conversation

@bradphelan
Copy link

@bradphelan bradphelan commented Nov 22, 2016

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
Copy link
Owner

kswoll commented Nov 22, 2016

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
Copy link
Author

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
#27 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABE8tS84OCOUClnxCWz48mHwobh3M5cks5rA0FjgaJpZM4K5Mqh
.

@kswoll
Copy link
Owner

kswoll commented Nov 29, 2016

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
Copy link
Author

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

@kswoll
Copy link
Owner

kswoll commented Nov 30, 2016

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

@bradphelan
Copy link
Author

bradphelan commented Nov 30, 2016 via email

bradphelan added 2 commits December 21, 2016 11:32
@IQuixote
Copy link

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
Copy link
Owner

kswoll commented Mar 31, 2017

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants