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

Negative property names should be discouraged #6131

Closed
1 of 2 tasks
BieleckiLtd opened this issue Jan 11, 2023 · 16 comments · Fixed by #8826
Closed
1 of 2 tasks

Negative property names should be discouraged #6131

BieleckiLtd opened this issue Jan 11, 2023 · 16 comments · Fixed by #8826
Labels
enhancement New feature or request epic

Comments

@BieleckiLtd
Copy link
Contributor

BieleckiLtd commented Jan 11, 2023

Feature request type

Other

Component name

No response

Is your feature request related to a problem?

This is because it is difficult to understand them when reading code. Instead of providing negative property names like Disabled it is much easier to think in positive forms, especially when there is a need to inverse the statement programmatically. In such cases, we end up with double negation which can be mind-bending. For example Disabled="false" is harder to understand than IsEnabled="true". Negated boolean properties such as NotEnabled, NotVisible must also be avoided, but thankfully I have not seen any in MudBlazor.

For anyone who would like to read more, I found a couple of interesting articles about this problem:

Describe the solution you'd like

Discourage Disabled, Hidden and similar property names. Promote IsEnabled, IsVisible etc. instead.

Have you seen this feature anywhere else?

Most API's follow this practice

Describe alternatives you've considered

No response

Pull Request

  • I would like to do a Pull Request

Code of Conduct

  • I agree to follow this project's Code of Conduct
@BieleckiLtd BieleckiLtd added enhancement New feature or request triage labels Jan 11, 2023
@BieleckiLtd BieleckiLtd changed the title Negative prefixes in property names should be discouraged Negative property names should be discouraged Jan 11, 2023
@Yomodo
Copy link
Contributor

Yomodo commented Jan 12, 2023

Nowadays I even prefer Enabled v.s. IsEnabled

@BieleckiLtd
Copy link
Contributor Author

BieleckiLtd commented Jan 22, 2023

Nowadays I even prefer Enabled v.s. IsEnabled

I'm finding similar discrepancies in MudBlazor all the time. Maybe these should be identified for rename in the next major breaking change?

As of using 'Is', 'Has', 'Can' ... in bool prop names - this is opinionated subject and intellisense to the rescue but if I'd had to vote, I'd say to keep them to avoid ambiguity. For example IsSet prop reads differently to me than just Set. Also I'm coming from WPF background (and now MAUI) and they both follow that convention and I'm just used to it.

The problem here is that there is no consequence in naming.

@dennisrahmen
Copy link
Contributor

You may want to look at the next milestone, the is basically the opposite of your proposal already on it.

@BieleckiLtd
Copy link
Contributor Author

You may want to look at the next milestone, the is basically the opposite of your proposal already on it.

Could you please share the link where I can find the next milestone plans? I've found https://github.com/MudBlazor/MudBlazor/projects/13#card-84986599, but it does seem to be aligned, right?

@dennisrahmen
Copy link
Contributor

Oh, sorry. Was sure it was the other way around. 🙈

@ScarletKuro
Copy link
Member

@henon What do you think?
I noticed that @Mr-Technician liked the post. Do you think this change should be implemented in v7? As someone coming from the WPF world, I am also used to IsEnabled, IsVisible etc practice. However, I feel it may be too late to address this. In v7 it could lead to an overwhelming number of changes.

@Mr-Technician
Copy link
Member

Another consideration I just thought of is that some negative property names reflect the underlying html attributes.

We could change Disabled to Enabled but the attribute on the elements is still called disabled. I not sure it's wise to introduce a disparity in those situations.

@henon
Copy link
Collaborator

henon commented Apr 9, 2023

Yes we are striving to get more consistency, positive property names and .Net-Compatible naming conventions like IsEnabled instead of Enabled. v7 will be a first step in the right direction. Most of what's about to change is already marked obsolete. We will probably mark more obsolete in v7 and break it in v8.

Another thing that really annoys me are event names like OnClick or OnKeyDown. In my opinion the event should be Click so you can name your handler OnClick. Now you have to name your handler OnClicked or OnKeyDownHandler which is suboptimal IMO. This would be a huge break however.

@ScarletKuro
Copy link
Member

Another thing that really annoys me are event names like OnClick or OnKeyDown.

I'm ok with this one, but as you mentioned, the "On-" prefix would be more appropriate for naming the event handler, rather than the event itself. However, I still believe that some kind of prefix or suffix should be used instead of using the bare "Click" name. "Click" looks like a property or attribute, and a name like "ClickEvent" (for example) would help distinguish it from properties.

@BieleckiLtd
Copy link
Contributor Author

BieleckiLtd commented Mar 23, 2024

I might be in minority here but imho naming events with prepositions and handlers with descriptive verbs that clearly outline their actions, such as OnClick=IncreaseCounter and AfterCountdownFinished=CloseApplication enhances code readability.

@henon
Copy link
Collaborator

henon commented Mar 25, 2024

@BieleckiLtd now is the time to make breaking changes. We have decided to make the next release v7.0.0

The problem is that we have no overview over all the little discrepancies and inconsistencies. If you like you could help cataloging them and we'd then decide on a renaming action.

@BieleckiLtd
Copy link
Contributor Author

I've also noticed that different components behave differently when their two-way bound parameters change. For example, when I use @bind-Toggled="true" with a MudToggleIconButton, it doesn't call ToggledChanged. But, when I use @bind-SelectedIndex="2" in a MudChart, it does call SelectedIndexChanged. This means some components let consumer know when their properties change, and others don't. Do we want to address that too? A couple of examples:

MudToggleIconButton

[Parameter] public bool Toggled { get; set; }
[Parameter] public EventCallback<bool> ToggledChanged { get; set; }

MudAutocomplete (note no Parameter on IsOpen prop)

[Parameter]
public EventCallback<bool> IsOpenChanged { get; set; }
private bool _isOpen;
public bool IsOpen
{
    get => _isOpen;
    protected set
    {
        if (_isOpen == value) return;
        _isOpen = value;
        IsOpenChanged.InvokeAsync(_isOpen).AndForget();
    }
}

MudBooleanInput

[Parameter] public EventCallback<T?> ValueChanged { get; set; }
protected T? _value;
[Parameter]
public T? Value
{
    get => _value;
    set => _value = value;
}

MudChart

[Parameter]
public int SelectedIndex
{
    get => _selectedIndex;
    set
    {
        if (value != _selectedIndex)
        {
            _selectedIndex = value;
            SelectedIndexChanged.InvokeAsync(value);
        }
    }
}
[Parameter] public EventCallback<int> SelectedIndexChanged { get; set; }

@ScarletKuro
Copy link
Member

ScarletKuro commented Mar 29, 2024

I've also noticed that different components behave differently when their two-way bound parameters change.

This problem will be gone once all components start to use ParameterState, the behavior will be consistent and same for all the components.

@danielchalmers
Copy link
Contributor

Do most web/Blazor projects prefer IsEnabled or Enabled style?

@BieleckiLtd
Copy link
Contributor Author

BieleckiLtd commented Mar 30, 2024

repo:dotnet/maui path:/src

1       Enabled
9       IsEnabled
0       Disabled
0       IsDisabled
0       Checked
6       IsChecked
0       Open
7       IsOpen
0       Visible
7       IsVisible
0       ReadOnly
39      IsReadOnly
0       Selected
0       IsSelected
0       Expanded
0       IsExpanded
0       Active
3       IsActive
0       Valid
0       IsValid

repo:dotnet/wpf path:/src/Microsoft.DotNet.Wpf/src

1       Enabled
3       IsEnabled
0       Disabled
0       IsDisabled
0       Checked
1       IsChecked
0       Open
0       IsOpen
0       Visible
0       IsVisible
1       ReadOnly
12      IsReadOnly
0       Selected
8       IsSelected
0       Expanded
0       IsExpanded
0       Active
1       IsActive
0       Valid
2       IsValid

repo:unoplatform/uno path:/src

1       Enabled
3       IsEnabled
0       Disabled
0       IsDisabled
0       Checked
0       IsChecked
0       Open
0       IsOpen
4       Visible
0       IsVisible
0       ReadOnly
42      IsReadOnly
1       Selected
1       IsSelected
0       Expanded
1       IsExpanded
0       Active
5       IsActive
0       Valid
0       IsValid

repo:AvaloniaUI/Avalonia path:/src

0       Enabled
2       IsEnabled
0       Disabled
0       IsDisabled
0       Checked
0       IsChecked
0       Open
0       IsOpen
0       Visible
0       IsVisible
0       ReadOnly
8       IsReadOnly
0       Selected
2       IsSelected
0       Expanded
0       IsExpanded
0       Active
8       IsActive
0       Valid
3       IsValid

repo:MudBlazor/MudBlazor path:/src

1       Enabled
2       IsEnabled
35      Disabled
1       IsDisabled
0       Checked
1       IsChecked
2       Open
0       IsOpen
6       Visible
4       IsVisible
18      ReadOnly
0       IsReadOnly
0       Selected
1       IsSelected
3       Expanded
8       IsExpanded
1       Active
1       IsActive
0       Valid
1       IsValid

Let me know if you'd like me to analyse other boolean properties or look at other repositories.

@danielchalmers
Copy link
Contributor

@henon Web frameworks:

Fluent UI is a mixed bag when deviating from the default

MUI like negative property names

AntBlazor prefer affirming names


They all seem to agree it should be Disabled instead of Enabled regardless of the other naming decisions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request epic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants