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 new constructor to AvaloniaDictionary and include unit tests #17311 #17312

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

IoannTerrible
Copy link
Contributor

#17311

What does the pull request do?

This pull request adds a new constructor to AvaloniaDictionary<TKey, TValue> that accepts an existing Dictionary<TKey, TValue>. The goal is to provide an easier and more efficient way to initialize AvaloniaDictionary with pre-existing dictionary data.

What is the current behavior?

Currently, AvaloniaDictionary lacks a constructor that directly accepts a Dictionary<TKey, TValue>. Developers have to manually copy elements into AvaloniaDictionary, which increases boilerplate code and is less efficient.

What is the updated/expected behavior with this PR?

With this PR, developers can initialize AvaloniaDictionary with an existing dictionary directly, simplifying the code and reducing manual effort.
To test:

  • You can create an AvaloniaDictionary by passing a populated dictionary and verify that all entries are correctly initialized.
  • Passing null should throw an ArgumentNullException.

Checklist

Breaking changes

No breaking changes are introduced, as this PR simply adds an overload to the existing constructors.

Obsoletions / Deprecations

Fixed issues

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052726-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented Oct 19, 2024

  • All contributors have signed the CLA.

@IoannTerrible
Copy link
Contributor Author

@cla-avalonia agree

@robloo
Copy link
Contributor

robloo commented Oct 21, 2024

Should the new constructor be an IDictionary interface rather than concrete class?

@MrJul MrJul added needs-api-review The PR adds new public APIs that should be reviewed. enhancement labels Oct 22, 2024
@MrJul
Copy link
Member

MrJul commented Jan 14, 2025

API diff for review:

namespace Avalonia.Collections
{
     public partial class AvaloniaDictionary {
     {
+        public AvaloniaDictionary(System.Collections.Generic.Dictionary<TKey, TValue> initialDictionary) { }
     }
}

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054275-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul MrJul removed the needs-api-review The PR adds new public APIs that should be reviewed. label Jan 21, 2025
Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

Notes from the API meeting:

As suggested in #17312 (comment), use the most complete constructor.

Expected API:
AvaloniaDictionary(IEnumerable<KeyValuePair<TKey,TValue>> collection, IEqualityComparer<TKey>? comparer);

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054395-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@MrJul MrJul added the api-approved The new public APIs have been approved. label Jan 21, 2025
Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

/// </summary>
public AvaloniaDictionary(IDictionary<TKey, TValue> dictionary, IEqualityComparer<TKey>? comparer = null)
{
if (dictionary != null)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you can remove this check and the throw below. Let the Dictionary's constructor throw on its own, as its documented to do so. There's no real point in checking the same thing several times.

@MrJul MrJul added this pull request to the merge queue Jan 23, 2025
Merged via the queue into AvaloniaUI:master with commit 58dacb0 Jan 23, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved The new public APIs have been approved. enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants