ref: revert accessibility change in AssemblyStoreReader#4868
ref: revert accessibility change in AssemblyStoreReader#4868
AssemblyStoreReader#4868Conversation
| internal TestAccessor GetTestAccessor() | ||
| => new(this); | ||
|
|
||
| internal readonly struct TestAccessor | ||
| { | ||
| private readonly StoreReader _instance; | ||
|
|
||
| internal TestAccessor(StoreReader instance) | ||
| { | ||
| _instance = instance; | ||
| } | ||
|
|
||
| internal bool IsSupported() => _instance.IsSupported(); | ||
| } |
There was a problem hiding this comment.
question: What do you think about Roslyn's TestAccessor Pattern in this case?
There was a problem hiding this comment.
Seems like there are pros and cons.
Pros:
- I like the fact that it's explicit about what is being exposed and why.
- It would allow us to leave some stuff private... although I can't remember a time when we got into trouble over the distinction between private and internal so I think in practice this is a negligible benefit
Cons:
- It is way more verbose than just changing the visibility of stuff to internal
I think we could do it for new code if we really wanted to. It's really a styling choice. For existing code, it's a whole lot of work for practically no benefit at all...
There was a problem hiding this comment.
Thanks for your feedback.
I removed the TestAccessor, and replaced it with a derived file-local type, as originally suggested in https://github.com/getsentry/sentry-dotnet/pull/4814/changes#r2707861556.
There was a problem hiding this comment.
Wouldn't that also sometimes require changing member visibility just for testing (e.g. private -> protected)? And we couldn't do this for sealed classes either.
What is the key problem we're trying to solve here?
- The risk of one of the maintainers using an internal member when they shouldn't?
- The lack of clarity around why some members are internal?
- Something else?
I don't think 1 is a serious concern... especially given our review process. 2 is usually handled by the compiler... if you try to make those internal members private, the tests won't compile anymore... 3 then?
It would be nice/pretty if C# had an attribute you could decorate members with to say they're public only to test assemblies... but short of that, I don't think there are any workarounds that don't come with both pros and cons. I think the approach we've been taking so far (using InternalsVisibleTo) is probably the best available alternative... it requires very little work, doesn't affect the source types at all for SDK users and has very little risk. It's not 'pretty' is about my only complaint with it but I think we have to be pragmatic.
There was a problem hiding this comment.
Fair point!
You convinced me.
Let's keep it as is.
Thanks for your feedback.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4868 +/- ##
=======================================
Coverage 73.85% 73.86%
=======================================
Files 483 483
Lines 17577 17577
Branches 3464 3464
=======================================
+ Hits 12982 12983 +1
Misses 3741 3741
+ Partials 854 853 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sentry review |
Flash0ver
changed the title
AssemblyStoreReader
2 participants
Follow-up to
I wanted to give The
TestAccessorPattern from Roslyn a try.See comments.
But changed to deriving a file-local type and use it in the test, as originally suggested.
We eventually decided against this change.
Making one member
internalis just less code to maintain than derived types and/or Test-Accessors.