-
Notifications
You must be signed in to change notification settings - Fork 297
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.
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
NonSerializable/XMLIgnore Attributes with ICommand/ObservableProperty attributes #208
Comments
Hello, 'Aurumaker72! Thanks for submitting a new feature request. I've automatically added a vote 👍 reaction to help get things started. Other community members can vote to help us prioritize this feature in the future! |
FYI @Sergio0694 this was in the WCT repo, @Arlodotexe we may want to touch base on Monday quick and look at our GitHub Issue categories or something and call out MVVM Toolkit better to redirect here, as I've been noticing a few issues still getting filed over for WCT. Also linking back to this comment here by @skint007 which was the same request:
|
Forwarded post /idea |
Bubbling up the solution proposed by @timunie:
That wouldn't account for if extra constructor arguments are needed though. Would we be able to use [AttributeDecorator(nameof(SerializeThing), "SomeOption = true", "SomeOtherOption = 3")]
private string _myField
// To Generate:
[SerializeThing(SomeOption = true, SomeOtherOption = 3)]
public string MyField { get; set; } Not sure if anything better than a raw string could be used... Then once dotnet/csharplang#3412 is implemented, this could just all go away in a future version? (but at least still be relatively simple to migrate back over?) |
As a related work item, I think we should try to get more traction on that proposal and talk to the Roslyn folks. cc. @333fred you can see not having access to partial properties is causing a significant amount of issues for us here. |
So, there is one big elephant in the room here: not all attributes are valid on both fields and properties. We need to remind ourselves that what we're doing today in the MVVM Toolkit is just a "best effort", but we'll just never be able to actually land on a "perfect" solution until C# actually gets support for partial properties (see dotnet/csharplang#3412). That said, we can try to make this at least a bit better. What we could do is: for all attributes on a field marked with Thoughts? 🙂 cc. @michael-hawker @Arlodotexe @Insire @timunie @powerdude, @sonnemaf? |
I like the idea above. Also, to complete the picture and for those scenarios where an attribute can't be on a field, but can be on a property, I think consideration should be made for the |
I am happy with whatever you choose. But tbh, I think "forwarding" an attribute may lead to unpredictable situations:
Happy coding |
Yes, that is what made me not do this in the first place. I do have many issues with this. Honestly I'm kinda leaning towards only doing so (especially since we're close to 8.0 stable), just saying that special atributes need a manual property for now, and then potentially revisit this post 8.0, or just push for getting partial properties in C# 12. |
Great points. I think these would lead to being explicit then and something like the |
I really don't like |
I agree with Sergio. Let's postpone this feature until there is a solid solution, instead of implementing a error prone one. |
Ok, fair enough. What about the use of a proxy class? Something like this:
|
Honestly I'm thinking the only proper way would be to have opt-in forwarded attributes, like: [ObservableProperty]
[SomeAttribute]
[SomeOtherAttribute]
[AlsoForwardAttribute(typeof(SomeAttribute))] // opt-in
private string name; This would instruct the generator to only forward What we might do is add this as a preview feature (ie. with |
Tbh I think postpone is the best we can do here. But just my two cents. |
@Sergio0694 in either case for now regardless of when we do this, should we have an info/warning diagnostic that if an "unrecognized"/un-auto-forwarded (i.e. non-validation) property is found that it calls that out? At least then there wouldn't be confusion about why something isn't working as expected. Something like:
Would it make sense to make it a negative case of |
This is now superseded by #413, closing this one. |
Describe the problem
A way/special attribute to be used in conjunction with ICommand or ObservableProperty which signalizes the backing field not to be serialized
Describe the solution
Example which should compile to
Alternatives
No response
Additional info
No response
Help us help you
No response
The text was updated successfully, but these errors were encountered: