Skip to content
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

Allow custom property attributes for [ObservableProperty] #413

Closed
Sergio0694 opened this issue Aug 31, 2022 · 8 comments · Fixed by #449
Closed

Allow custom property attributes for [ObservableProperty] #413

Sergio0694 opened this issue Aug 31, 2022 · 8 comments · Fixed by #449
Assignees
Labels
feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit

Comments

@Sergio0694
Copy link
Member

Sergio0694 commented Aug 31, 2022

Overview

Right now, it's not possible to specify property attributes when using [ObservableProperty], which has been a pain point for some users, as it forces you to go back to manual properties in that case. There's been several asks for this in the past, see (#228, #217, #208). We could technically support this via the explicit property: target, which Roslyn ignores but doesn't block. That is:

[ObservableProperty]
[property: JsonPropertyName("name")]
private string? name;

The generator would emit:

[JsonPropertyName("name")]
public string? Name
{
    get => name;
    set => ...
}

This would effectively allow users to have perfect control over attributes on target properties.

Note: Roslyn will currently emit a diagnostic in this case, so users would have to suppress it or just ignore it. Just not the perfect tooling experience, but it would still solve the issue and be a valid solution for now with no language changes needed. which we can automatically suppress with a dedicated diagnostic suppressor, so not a problem.

@Sergio0694 Sergio0694 added feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit labels Aug 31, 2022
@Sergio0694 Sergio0694 self-assigned this Aug 31, 2022
@Sergio0694
Copy link
Member Author

@333fred I know you were doing some work to block uses of invalid attribute targets, was that restricted to not recognized targets (ie. CS0658), or would that include invalid targets (ie. CS0657) too? As in, could we rely on that not being an error?

@Sergio0694
Copy link
Member Author

Additionally (cc. @chsienki) would there be a way for a generator to suppress CS0657 on that field on behalf of users? As in, a way for it to basically tell Roslyn that yes those are ignored, but the generator actually needs that info, so it shouldn't be a warning. Being able to suppress those CS0657 from the generator would make the UX much better for the end users 🙂

@chsienki
Copy link

@Sergio0694 The generator can't do that, but you can ship a diagnostic suppressor in the same assembly. They were basically invented for this purpose :) https://github.com/dotnet/roslyn/blob/32c1f2d2df534f9ae3679807ecdfdd0706e8da6d/docs/analyzers/DiagnosticSuppressorDesign.md

@Sergio0694
Copy link
Member Author

Ooh that's awesome, thank you! I'll look into it, this seems very promising then 😄

@TruePluto
Copy link

”which Roslyn ignores but doesn't block.“ Does Mean “It's just a makeshift or an expedient”
thanks

@virzak
Copy link

virzak commented Feb 9, 2023

Should this be available on RelayCommand?

[RelayCommand]
[property: Newtonsoft.Json.JsonIgnore]
private void DoWork() { }

When I try to serialize an object which contains this code, DoWork gets serialized and this is something I'd like to avoid.

@Sergio0694
Copy link
Member Author

This is doable, though would need additional justification for the work. Could you open a dedicated proposal?
Thank you! 🙂

@Aurumaker72
Copy link

Should this be available on RelayCommand?

[RelayCommand]
[property: Newtonsoft.Json.JsonIgnore]
private void DoWork() { }

When I try to serialize an object which contains this code, DoWork gets serialized and this is something I'd like to avoid.

While this is not related to the toolkit, I'd like to add that serializing ViewModels is usually a sign of some misstep.

Consider serializing the backing model, which is often a trivially serializable DTO or POCO, instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants