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

Inherited ViewModels can hide important warnings #451

Open
1 of 4 tasks
MisinformedDNA opened this issue Sep 20, 2022 · 3 comments · May be fixed by #773
Open
1 of 4 tasks

Inherited ViewModels can hide important warnings #451

MisinformedDNA opened this issue Sep 20, 2022 · 3 comments · May be fixed by #773
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit

Comments

@MisinformedDNA
Copy link

Describe the bug

If an ObservableProperty is created in two ViewModels, a parent and child, then the parent property gets hidden and the warning is blocked from the user.

Regression

No response

Steps to reproduce

For this code:


public partial class ParentViewModel : ObservableObject
{
    [ObservableProperty] private int _someValue;
    public int AnotherValue { get; set; }
}

public partial class DerivedViewModel : ParentViewModel
{
    [ObservableProperty] private int _someValue;
    public int AnotherValue { get; set; }
}


The following warning is generated:

> 'DerivedViewModel.AnotherValue' hides inherited member 'ParentViewModel.AnotherValue'. Use the new keyword if hiding was intended.

Expected behavior

The following warning should also be generated:

'DerivedViewModel.SomeValue' hides inherited member 'ParentViewModel.SomeValue'. Use the new keyword if hiding was intended.

Screenshots

No response

IDE and version

VS 2022

IDE version

17.3.0

Nuget packages

  • CommunityToolkit.Common
  • CommunityToolkit.Diagnostics
  • CommunityToolkit.HighPerformance
  • CommunityToolkit.Mvvm (aka MVVM Toolkit)

Nuget package version(s)

8.0.0

Additional context

This is due to warnings being disabled with

#pragma warning disable

Fix options:

  1. The pragma is enabled for this warning

    • Pros: Warning might alert the user to unintended functionality
    • Cons: User can't suppress the warning
  2. Option 1 plus ObservableProperty is modified to accept a parameter that says whether hiding is desired

    • Pros: The warning is exposed and can be suppressed
    • Cons: May not be worth the complication

Help us help you

Yes, I'd like to be assigned to work on this item

@MisinformedDNA MisinformedDNA added the bug 🐛 An unexpected issue that highlights incorrect behavior label Sep 20, 2022
@mikechristiansenvae
Copy link

mikechristiansenvae commented Oct 3, 2022

I'm not understanding why this would be considered a bug with CommunityToolkit.Mvvm. The warning that is produced is accurate.


I'm trying to understand why you want to do this? Hiding properties and stuff is almost certainly a bad idea.

Since you're not changing the generated properties or anything, the derived class doesn't need to re-define the properties.

Why not simply do this:

public partial class ParentViewModel : ObservableObject
{
    [ObservableProperty] private int _someValue;
    public int AnotherValue { get; set; }
}

public partial class DerivedViewModel : ParentViewModel 
{
}

@MisinformedDNA
Copy link
Author

The warning that is produced is accurate.

@mikechristiansenvae The bug is that the warning is not produced. And like you said, "Hiding properties and stuff is almost certainly a bad idea." I'm saying that the warning should be produced.

@Sergio0694 Sergio0694 added the mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit label Oct 3, 2022
@mikechristiansenvae
Copy link

mikechristiansenvae commented Oct 5, 2022

Ah! Fair point!

I agree, you are correct. (Additionally, I've replicated the issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit
Projects
None yet
3 participants