[Fix] ReflectionExtensions Performance Improvments#3991
Conversation
and make Paramore.Brighter InternalsVisibleTo to Paramore.Brighter.Core.Tests
SimonCropp
requested review from
DevJonny,
holytshirt,
iancooper and
preardon
as code owners
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
PR Review: ReflectionExtensions Performance Improvements
Summary
This PR simplifies the ReflectionExtensions methods by leveraging built-in .NET reflection APIs more effectively, resulting in cleaner code and better performance.
Changes Overview
1. ReflectionExtensions.cs - Good simplifications:
GetOtherHandlersInPipeline(),GetOtherWrapsInPipeline(),GetOtherUnwrapsInPipeline()→ Replaced verbose LINQ queries with directGetCustomAttributes<T>(true)callsHasNoInboxAttributesInPipeline(),HasExistingUseInboxAttributesInPipeline()→ ReplacedAny()withIsDefined()which is more efficient as it avoids attribute instantiation
2. Infrastructure changes for testing internal methods:
- Added
InternalsVisibleToin the main project - Added strong-name signing to test assemblies
THIS IS NOT ALLOWED. We do not expose internal classes to tests; only public ones. We seek to build deep modules with narrow interfaces. Don't just make ReflectionExtensions public to fix this; it is an implementation detail and should not have its own tests
Verdict
The changes look good - the code is cleaner, more idiomatic, and the behavioral change to support deeper inheritance hierarchies is an improvement.
We MUST fix the test issues
iancooper
left a comment
iancooper
left a comment
There was a problem hiding this comment.
Whilst the performance improvements to the ReflectionExtensions are welcome, other aspects of the PR cannot be accepted because they violate our guidelines on testing.
iancooper
changed the title
…ons-perf-improvments
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Thanks @SimonCropp |
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
) * add ReflectionExtensions tests and make Paramore.Brighter InternalsVisibleTo to Paramore.Brighter.Core.Tests * Update ReflectionExtensionsTests.cs * Update ReflectionExtensions.cs * Update ReflectionExtensions.cs * more tests and simplify ReflectionExtensions * . --------- Co-authored-by: Ian Cooper <ian_hammond_cooper@yahoo.co.uk>
3 participants
No description provided.