Skip to content

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

Closed
wants to merge 2 commits into from
Closed

LookupType returns the first type found #622

wants to merge 2 commits into from

Conversation

Cronan
Copy link
Contributor

@Cronan Cronan commented Feb 7, 2018

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.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@den-run-ai
Copy link
Contributor

den-run-ai commented Feb 7, 2018

@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.

@Cronan
Copy link
Contributor Author

Cronan commented Feb 7, 2018

@denfromufa The issue in #613 from System import Environment is caused by this bug - it occurs in Linux using .Net Core 2.0, when using the code from #612.

I'll add a test that exercises the import above, but it will only fail when #612 is merged into master (and if the above code isn't added)

@Cronan Cronan changed the title LookupType returns public types first LookupType returns the first type found Feb 7, 2018
@Cronan Cronan mentioned this pull request Feb 7, 2018
@codecov
Copy link

codecov bot commented Feb 7, 2018

Codecov Report

Merging #622 into master will increase coverage by 1.91%.
The diff coverage is 100%.

Impacted file tree graph

@@           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
Flag Coverage Δ
#setup_linux 69.42% <ø> (ø) ⬆️
#setup_windows 76.18% <100%> (+2.11%) ⬆️
Impacted Files Coverage Δ
src/runtime/assemblymanager.cs 90.05% <100%> (+1.34%) ⬆️
src/runtime/runtime.cs 90.98% <0%> (-0.76%) ⬇️
src/runtime/interop.cs 95.73% <0%> (-0.49%) ⬇️
src/runtime/pythonengine.cs 78.31% <0%> (-0.26%) ⬇️
src/runtime/interop33.cs 100% <0%> (ø)
src/runtime/interop34.cs 100% <0%> (ø)
src/runtime/interop35.cs 100% <0%> (ø)
src/runtime/interop36.cs 100% <0%> (ø)
src/runtime/typemanager.cs 84.03% <0%> (+0.34%) ⬆️
src/runtime/moduleobject.cs 85.05% <0%> (+0.57%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8629e1e...1cfc844. Read the comment docs.

@den-run-ai den-run-ai mentioned this pull request Feb 7, 2018
@Cronan
Copy link
Contributor Author

Cronan commented Feb 7, 2018

@denfromufa Added a test, merged from #621, and squashed.

@den-run-ai
Copy link
Contributor

@Cronan let's wait for CI to finish

@Cronan
Copy link
Contributor Author

Cronan commented Feb 7, 2018

@denfromufa Sorry (sadface)

@dmitriyse
Copy link
Contributor

dmitriyse commented Feb 7, 2018

I also have related PR #528

@dmitriyse
Copy link
Contributor

That exists since summer 2017.

@den-run-ai
Copy link
Contributor

@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.

@Cronan
Copy link
Contributor Author

Cronan commented Feb 8, 2018

@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?

@den-run-ai
Copy link
Contributor

@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.

@den-run-ai
Copy link
Contributor

@Cronan also checkout the featured branch:

https://github.com/pythonnet/pythonnet/tree/featured

@Cronan
Copy link
Contributor Author

Cronan commented Feb 8, 2018

Thanks @denfromufa

@Cronan
Copy link
Contributor Author

Cronan commented Feb 12, 2018

@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.
@dmitriyse I'm not sure #528 actually address the issue of importing System.Environment in dotnetcoreapp2.0 on Linux, but I may be wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants