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.

Sign up for GitHub

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

Jump to bottom

KeyedCollection debug view doesn't work #96261

Merged
merged 6 commits into from
Jan 12, 2024
Merged

KeyedCollection debug view doesn't work #96261

merged 6 commits into from
Jan 12, 2024

Conversation

pedrobsaila
Copy link
Contributor

Fixes #96116

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 21, 2023
@ghost
Copy link

ghost commented Dec 21, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #96116

Author: pedrobsaila
Assignees: -
Labels:

area-System.Collections
Milestone: -

This was referenced Dec 22, 2023
Closed
Closed
@build-analysis build-analysis bot mentioned this pull request Jan 4, 2024
Closed
This was referenced Jan 5, 2024
Open
Closed
@eiriktsarpalis
Copy link
Member

@pedrobsaila before we get this merged, has the new proxy type been tested? I believe we write unit tests for a few other collections types, for example this one:

https://github.com/dotnet/runtime/blob/c31a2173e5a8b2cdef94b986504cf23b9a6bb175/src/libraries/Common/tests/System/Collections/DebugView.Tests.cs

@pedrobsaila
Copy link
Contributor Author

Tested it only locally inside VS, did not make a unit test yet.

@eiriktsarpalis
Copy link
Member

Would you be ok adding a unit test in the mold of what I shared from collections?

@pedrobsaila
Copy link
Contributor Author

pedrobsaila commented Jan 9, 2024 •
edited
Loading

Would you be ok adding a unit test in the mold of what I shared from collections?

yes for sure. I'll push the tests whithin a few hours

eiriktsarpalis
eiriktsarpalis reviewed Jan 11, 2024
View reviewed changes
src/libraries/Common/tests/System/Collections/DebugView.Tests.cs
{
}

public Func<TValue, TKey> GetKeyForItemHandler { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think some of that indirection could be removed if you just made the derived type non-generic and inlined the GetKeyBehavior in the implementation.

eiriktsarpalis
eiriktsarpalis approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

LGTM. I left some feedback on a minor nitpick on the test, but it's up to you if you want to address it. Thanks!

@eiriktsarpalis eiriktsarpalis merged commit 525a6d7 into dotnet:main Jan 12, 2024
111 checks passed
@pedrobsaila pedrobsaila deleted the 96116 branch January 12, 2024 15:24
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@pedrobsaila @tmds
KeyedCollection debug view doesn't work (dotnet#96261)
c993d57 
* KeyedCollection debug view doesn't work

* fix remarks 1

* fix remarks 2

* add tests
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@JamesNK JamesNK JamesNK left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

Assignees
No one assigned
Labels
area-System.Collections community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

KeyedCollection debug view doesn't work
4 participants
@pedrobsaila @eiriktsarpalis @JamesNK @stephentoub