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

feat: (mvux) Add new UpdateItemasync Extension Method #2642

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ajpinedam
Copy link
Contributor

GitHub Issue (If applicable): closes #2582

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

To update an item, we need to do something like ListState.UpdateAllAsync(t => t.KeyEquals(newT), newT);

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added/updated which fit documentation template. (for bug fixes / features)
  • Unit Tests and/or UI Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Wasm UI Tests are not showing unexpected any differences. Validate PR Screenshots Compare Test Run results.
  • Contains NO breaking changes
  • Updated the Release Notes
  • Associated with an issue (GitHub or internal)

Other information

Internal Issue (If applicable):

@ajpinedam ajpinedam requested a review from dr1rrb December 11, 2024 04:05
@ajpinedam ajpinedam self-assigned this Dec 11, 2024
/// <param name="newT">The new value for the item.</param>
/// <param name="ct">A token to abort the async add operation.</param>
/// <returns></returns>
public static ValueTask UpdateItemAsync<T>(this IListState<T> state, T oldT, T newT, CancellationToken ct = default) where T : IKeyEquatable<T>
Copy link
Member

@dr1rrb dr1rrb Jan 7, 2025

Choose a reason for hiding this comment

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

To be consistent with other methods like this (e.g. State.SetAsync) in order to protect ACID properties, I would add a constraint on T : struct (and add overloads with T? and string).

Also not a fan of the names of the parameters, I would rename those:

Suggested change
public static ValueTask UpdateItemAsync<T>(this IListState<T> state, T oldT, T newT, CancellationToken ct = default) where T : IKeyEquatable<T>
public static ValueTask UpdateItemAsync<T>(this IListState<T> state, T oldItem, T newItem, CancellationToken ct = default) where T : IKeyEquatable<T>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dr1rrb Added a new overload and also updated the parameter names.

src/Uno.Extensions.Reactive/Core/ListState.Extensions.cs Outdated Show resolved Hide resolved
@ajpinedam ajpinedam force-pushed the feat/mvux.updateitemasync branch from 2983601 to 4fe5eca Compare January 9, 2025 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need ListState.UpdateItemAsync
2 participants