Skip to content

MudBreadcrumbs: Convert BreadcrumbItem to record type#10116

Merged
henon merged 1 commit intoMudBlazor:devfrom
danielchalmers:breadcrumb-item-record
Nov 2, 2024
Merged

MudBreadcrumbs: Convert BreadcrumbItem to record type#10116
henon merged 1 commit intoMudBlazor:devfrom
danielchalmers:breadcrumb-item-record

Conversation

@danielchalmers
Copy link
Copy Markdown
Member

Description

How Has This Been Tested?

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.18%. Comparing base (bacf629) to head (7489ce3).
Report is 48 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev   #10116   +/-   ##
=======================================
  Coverage   91.17%   91.18%           
=======================================
  Files         411      411           
  Lines       12481    12483    +2     
  Branches     2429     2432    +3     
=======================================
+ Hits        11380    11382    +2     
  Misses        557      557           
  Partials      544      544           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielchalmers danielchalmers added the breaking change This change will require consumer code updates (ex: removes/changes a public API) label Oct 29, 2024
@ScarletKuro ScarletKuro requested a review from henon October 31, 2024 22:05
@ScarletKuro
Copy link
Copy Markdown
Member

ScarletKuro commented Oct 31, 2024

Wouldn't it better to make just

public record BreadcrumbItem(string Text, string? Href, bool Disabled = false, string? Icon = null);

?

@danielchalmers
Copy link
Copy Markdown
Member Author

Wouldn't it better to make just

public record BreadcrumbItem(string Text, string? Href, bool Disabled = false, string? Icon = null);

?

That's a breaking change if you reference the variable name in the constructor: new BreadcrumbItem(href: "..."). There's no href anymore.

@ScarletKuro
Copy link
Copy Markdown
Member

ScarletKuro commented Nov 1, 2024

That's a breaking change if you reference the variable name in the constructor: new BreadcrumbItem(href: "..."). There's no href anymore.

That's very interesting, I thought the generated code would lowercase it? But seems like I'm wrong

public BreadcrumbItem([Nullable(1)] string Text, string Href, bool Disabled = false, string Icon = null)
{
            <Text>k__BackingField = Text;
            <Href>k__BackingField = Href;
            <Disabled>k__BackingField = Disabled;
            <Icon>k__BackingField = Icon;
            base..ctor();
}

No idea why would they do that, considering that record is just a "superstructure" over class.
Ty, learned something new today.

Copy link
Copy Markdown
Contributor

@henon henon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change? What to write in the migration guide?

@ScarletKuro
Copy link
Copy Markdown
Member

ScarletKuro commented Nov 1, 2024

I don't think it's a breaking change either? The properties were readonly already, all you added is that with record it implements ToString and Equality etc which is a good thing.

@danielchalmers
Copy link
Copy Markdown
Member Author

@ScarletKuro @henon Breaks inheritance because it's no longer a class.

CS8865: Only records may inherit from records.

Migration guide could say

BreadcrumbItem is now a record and any class that inherits from it must be updated to a record as well

@henon henon changed the title Breadcrumbs: Convert BreadcrumbItem to record type MudBreadcrumbs: Convert BreadcrumbItem to record type Nov 2, 2024
@henon henon merged commit 6938bbb into MudBlazor:dev Nov 2, 2024
@henon henon mentioned this pull request Nov 2, 2024
@henon
Copy link
Copy Markdown
Contributor

henon commented Nov 2, 2024

Added to v8.0.0 Migration Guide
#9953

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change This change will require consumer code updates (ex: removes/changes a public API)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants