Avoid LINQ Count method - #13545
Conversation
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment. |
|
xtqqczze Is it still draft? |
97d1459 to
f1b25ee
Compare
5571611 to
3b2c5a3
Compare
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
Rob Holt (@rjmholt) Can you review the changes since your last review? |
Rob Holt (rjmholt)
left a comment
There was a problem hiding this comment.
xtqqczze you didn't make the changes I asked for last time, your new refactor incorrectly changes the behaviour of the code, and there's no testing added to ensure lack of regressions.
Given the quantity of open PRs you have on PowerShell right now, I would ask that you:
- Refocus your contributions for quality rather than quantity
- Add tests in your contributions to enable us as maintainers to make the case that your contributions won't cause regressions
Please understand that this is a large and relatively old codebase, and due to its use as a programming language and library platform that's reflection-intensive, it is a lot more sensitive to regressions than many other .NET projects, meaning the maintainers try to be conservative with code mutations that aren't strictly necessary.
I'm happy to accept your contributions, but those contributions should:
- Be well-scoped, both in terms of the domain and the code
- Make a strong case for the proposed change in the PR description, about improving some factor like performance, code complexity or testing coverage, and ideally make it in a quantitative way
- Add tests or identify in the PR description where testing pre-exists to identify regressions
For this PR, please:
- Change the
GetEnumerator()usage to aforeach - Add a test for the code path you're refactoring
Address @rjmholt review Co-Authored-By: Robert Holt <rjmholt_msft@outlook.com>
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
🎉 Handy links: |
PR Summary
Refactor code to avoid LINQ Count method.
DoesCurrentRunspaceIncludeCoreHelpCmdletmethodRefactor method parameters to a type that implements
ICollection<T>(for the Count property)IEnumerable<TypeMemberData>->Dictionary<string, TypeMemberData>.ValueCollectionIEnumerable<PropertySetData>->List<PropertySetData>I choose not to use a
ICollection<T>type because the enumerator may result in a heap allocation.PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.