NotifyAutoPropertiesInConstructorAttribute

[tom-englert]

This extends #220 with the possibility to opt-in for events from within the constructor by adding an attribute.

[navozenko]

I think that the NotifyAutoPropertiesInConstructorAttribute is not needed. I believe that this should be the default behavior and I suggest returning version 2.1.1. In my opinion, it is illogical when setters of different types of properties behave differently in constructors. This is a potential source of errors: #265

[cervengoc]

+1 on @navozenko's opinion, please restore the "normal" behavior as default, and make this constructor-rewriting feature optional. And in any case, please create some kind of release notes, because it's really hard to track the changes.

[tom-englert]

This will now fix #265, suppressing constructor notifications is now optional.

[navozenko]

@tom-englert excuse me, but in my opinion, it is a bad idea if auto-properties behave differently than other properties. This is a source of confusion. In addition, this does not solve the problem of virtual methods, because for custom properties notifications will still be sent.

I think that the notification for all properties should be uniform: either all properties notify, or all are not.

[tom-englert]

@navozenko for old-style properties with backing fields you would usually initialize the backing field in the constructor, not the property. With auto-properties you have no chance to do so - so they already behave different by design. And with Fody in general many things behave different than you would expect if you are just looking at the code, so you really have to know what you are doing.

Reading the discussions and seeing how people are using this tool, I agree that the default behavior should not change; this PR already reflects this, you now need to explicitly opt-in if you need this feature.

[navozenko]

@tom-englert sorry, I can not agree with you. To initialize custom properties in a constructor, I prefer to use the properties themselves, because the setter can do more than just write the value in the field. Even if now the setter does not do anything important, I prefer that my program does not break, if functionality will be added to the setter in the future.

In addition, if you agree to keep the previous default behavior, then the presence of the NotifyAutoPropertiesInConstructor attribute looks strange. For me it would be more understandable if you named your attribute as DoNotNotifyAutoPropertiesInConstructor without parameters.

And yet, I suggest that we consider a more general and uniform approach. For example, DoNotNotifyInConstructor attribute, which would disable notifications for all properties in the constructor.

[tom-englert]

There's nothing to agree with - there are just different preferences.

Initializing the property from within the constructor can be dangerous, because the object is in an undetermined state, and the setter would have to deal with this. This is similar to CA2214. Therefore adding logic to the setter should be strictly avoided, so it does not depend on being called from within the constructor or not.

However you seem follow a different paradigm, which is OK for you, but not an option for us.

NotifyAutoPropertyInConstructor with a parameter gives you more flexibility, than just a DoNotNotifyAutoPropertyInConstructor. You can e.g. opt-out at assembly level, but again op-in for a certain class, constructor or even property. I always prefer the general design.

[navozenko]

@tom-englert

Initializing the property from within the constructor can be dangerous, because the object is in an undetermined state, and the setter would have to deal with this. This is similar to CA2214.

In my opinion these are different situations. When properties are not virtual, there is no danger of calling them from the constructor. In addition, we aim not to conduct a discussion about a best programming practices, but to make a helper tool that has a uniform and intuitively expected behavior.

NotifyAutoPropertyInConstructor with a parameter gives you more flexibility, than just a DoNotNotifyAutoPropertyInConstructor.

I believed that DoNotNotifyAutoPropertyInConstructor is just an inversion of your attribute with the same functionality. Just the name DoNotNotifyXXXX fits better with the terminology of PropertyChanged.Fody. We must be sequential in naming. Now the attribute AlsoNotifyFor is used where events are not raised by default, and the DoNotNotify attribute is used where events are raised by default. Since your attribute is only needed to suppress events, it's more correct to name it as DoNotNotifyXXXX.

But I repeat, in my opinion, the most correct thing would be to add a common attribute like a DoNotNotifyInConstructor to suppress all notifications, and not just for auto-properties.

[tom-englert]

So to notify you would say DoNotNotify(false) and to not notify DoNotNotify(true)? That sounds weird to me.

[cervengoc]

What about configuring this feature in FodyWeavers.xml? To me it doesn't make too much sense to have several classes where one would like it, and several others where not. This feature rather seems to me like kind of a global thing which controls the overall behavior in this sense.

[navozenko]

@tom-englert

So to notify you would say DoNotNotify(false) and to not notify DoNotNotify(true)?

To notify I should not say anything. To not notify I would only say [DoNotNotify].

[tom-englert]

@cervengoc: In this case just put the attribute on assembly level: [assembly: Notify....]. This has the advantage that you can put it into a shared file and apply to the whole solution, instead of mangling with every projects FodyWeavers.xml.
However a configuration via FodyWeavers.xml could be added easily.

[tom-englert]

@navozenko so if I say [assembly: DoNotNotify] how could I switch on notification for a specific class? This would then require another attribute.

[navozenko]

From this point of view, perhaps you are right. but I'm not sure that putting this attribute on assembly is a good idea. Perhaps it's better to explicitly specify this attribute for each class, because it makes the class behavior unexpected and nonstandard.

[tom-englert]

@navozenko that's why I've chosen the current design: you are free to apply it on whatever level you think it's appropriate according to your personal preferences.

[cervengoc]

@tom-englert I agree with @navozenko about the importance of naming conventions. I think the DoNotNotify attribute could be appliable on constructors which could mean turning this feature on.

But I think hacking with assignments is not the good way,. I like the not-notified context concept much more as @navozenko prepared it in his proposal. Then anyone could apply this DoNotNotify attribute to any method or constructor, and it would mean that property assignments within that method would not trigger a notification.

[navozenko]

@cervengoc

I like the not-notified context concept much more as @navozenko prepared it in his proposal.

Thank you. Unfortunately, I had to delete my example, because it was not well thought out and could cause problems with multithreading.

If we suppress notifications in a constructor only, then there is no multithreading problem and we can use the flag that notifies all properties to enable/disable of notifications.

[tom-englert]

I will create a new PR after #267 has been merged, so it is proven that it does not break the baseline.