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

Add remapping extensions for core #17918

Closed
wants to merge 5 commits into from
Closed

Add remapping extensions for core #17918

wants to merge 5 commits into from

Conversation

PureWeen
Copy link
Member

@PureWeen PureWeen commented Oct 9, 2023

Description of Change

This PR adds a set of extension methods that we can use to ReMap mapper delegates inside core. This models the remapping mechanisms that we've already added to controls. Various scenarios come up where we just want to inject some additional logic into a subclassed handler but we still want the code from the base handler to run. This also ensures that if the user has replaced logic at the ViewHandler level, it will continue to run.

TODO

  • Extension methods need better naming, but I didn't want to lose the end result of API discussions.

Issues Fixed

#17665

@PureWeen PureWeen changed the title Add remapping extentions for core Add remapping extensions for core Oct 9, 2023
@samhouts samhouts added this to the Under Consideration milestone Oct 9, 2023
@jsuarezruiz jsuarezruiz added area-controls-image t/bug Something isn't working labels Oct 10, 2023
@ghost ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Oct 10, 2023
@PureWeen PureWeen removed the t/bug Something isn't working label Oct 10, 2023
@PureWeen PureWeen marked this pull request as ready for review October 10, 2023 14:37
@PureWeen PureWeen requested a review from a team as a code owner October 10, 2023 14:37
@PureWeen PureWeen requested review from jfversluis, rmarinho, hartez and mattleibow and removed request for jfversluis and rmarinho October 10, 2023 14:37
@PureWeen PureWeen marked this pull request as draft October 10, 2023 23:10
src/Core/src/PropertyMapperExtensions.cs Outdated Show resolved Hide resolved
src/Core/src/PropertyMapperExtensions.cs Outdated Show resolved Hide resolved
src/Core/src/PropertyMapperExtensions.cs Outdated Show resolved Hide resolved
src/Core/src/CommandMapperExtensions.cs Show resolved Hide resolved
@samhouts samhouts added the s/pr-needs-author-input PR needs an update from the author label Oct 16, 2023
@ghost
Copy link

ghost commented Oct 16, 2023

Hi @PureWeen. We have added the "s/pr-needs-author-input" label to this issue, which indicates that we have an open question/action for you before we can take further action. This PRwill be closed automatically in 14 days if we do not hear back from you by then - please feel free to re-open it if you come back to this PR after that time.

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

I think this is much better - and I am 60% sure that changing the return type is both breaking but not really.

Comment on lines 15 to 16
public static void ModifyMapping<TVirtualView, TViewHandler>(this IPropertyMapper<TVirtualView, TViewHandler> propertyMapper,
#pragma warning disable RS0016 // Add public types and members to the declared API
public static IPropertyMapper<TVirtualView, TViewHandler> ModifyMapping<TVirtualView, TViewHandler>(this IPropertyMapper<TVirtualView, TViewHandler> propertyMapper,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is breaking... I think methods can't be overloaded just by return type, so this may actually work. I'll test and report back.

[nameof(ISwipeView.TopItems)] = MapTopItems,
[nameof(ISwipeView.RightItems)] = MapRightItems,
[nameof(ISwipeView.BottomItems)] = MapBottomItems,
public static IPropertyMapper<ISwipeView, ISwipeViewHandler> Mapper =
Copy link
Member

Choose a reason for hiding this comment

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

There is an alternative that we could do and not have more extension methods:

public static IPropertyMapper Mapper = new()
{
    [nameof(Xxx)] = MapXxx,
}
.Remap(mapper =>
{
    mapper.ModifyMapping(xxx, yyy);
    mapper.ReplaceMapping(xxx, yyy);
});

@ghost
Copy link

ghost commented Oct 28, 2023

Hi @PureWeen.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

@ghost ghost added the stale Indicates a stale issue/pr and will be closed soon label Oct 28, 2023
@ghost ghost closed this Nov 1, 2023
@hartez
Copy link
Contributor

hartez commented Nov 1, 2023

Chill out, robot - we're thinking.

@hartez hartez reopened this Nov 1, 2023
@ghost ghost closed this Nov 6, 2023
@PureWeen PureWeen reopened this Nov 6, 2023
@ghost ghost removed stale Indicates a stale issue/pr and will be closed soon s/pr-needs-author-input PR needs an update from the author labels Nov 6, 2023
@spadapet
Copy link
Contributor

I have this open PR:

My PR might be blocked on this draft PR. But if mine is completed first, I'll comment here that its fix needs to be adjusted.

@Redth Redth removed this from the .NET 8 SR1 milestone Nov 30, 2023
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Dec 14, 2023
@hartez hartez removed their request for review January 10, 2024 20:18
@PureWeen PureWeen closed this May 1, 2024
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-image stale Indicates a stale issue/pr and will be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants