-
Notifications
You must be signed in to change notification settings - Fork 747
LookupType returns the first type found #622
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
Conversation
@Cronan i can't find any reference to this particular issue, i understand that writing unit test for this is hard, but please provide a reference comment or exception traceback at least. |
Codecov Report
@@ Coverage Diff @@
## master #622 +/- ##
========================================
+ Coverage 75.09% 77% +1.91%
========================================
Files 60 64 +4
Lines 5480 5614 +134
Branches 864 890 +26
========================================
+ Hits 4115 4323 +208
+ Misses 1064 1002 -62
+ Partials 301 289 -12
Continue to review full report at Codecov.
|
@denfromufa Added a test, merged from #621, and squashed. |
@Cronan let's wait for CI to finish |
@denfromufa Sorry (sadface) |
I also have related PR #528 |
That exists since summer 2017. |
@dmitriyse this PR addresses one stand-alone issue, I'm in favor of merging your PR, if it totally covers this case and if you cherry pick the unit test from this PR to your PR. |
@dmitriyse @denfromufa It seems I spent a lot of time yesterday debugging and fixing an issue that has already been fixed, but was left unreviewed and unmerged. :-( What is hard about this project right now is the large number of issues, combined with a large number of unreviewed and unmerged PRs, which make it increasingly difficult for new people to work on the project. Obviously #610 was meant to address this, but I think we also need to work through the PRs, and either merge, fix, or close them. Maybe we could take them one at a time, and focus in on them? |
@Cronan with new rules in issue #610 we should be able to faster merge existing PRs at least between me and @dmitriyse. I have more time this year than in second half of 2017. Soon we may add more reviewers from active people with few accepted PRs. |
@Cronan also checkout the featured branch: |
Thanks @denfromufa |
@denfromufa I'm going to close this, I'll add the test (with a few others) by itself as a separate PR to extend code coverage. |
What does this implement/fix? Explain your changes.
In .Net Core 2.0, there are public and private versions of the same types in different assemblies. For example, there is a private version of System.Environment in mscorlib.dll, and a public version in System.Runtime.Extensions.dll. The existing code returns the first matching type found, even if it's private, and even if there are other matching types in other assemblies.
The new version of this function returns the first public type, or the first type found.
Does this close any currently open issues?
This closes some parts of #613
Checklist
Check all those that are applicable and complete.
AUTHORS
CHANGELOG