Use GetExportedTypes where possible and filter nested types #723
Conversation
This is basically what @dmitriyse prepared in PR 528 without the event code that I don't understand.
|
@denfromufa Any idea for a test? |
|
@filmor i don't have a good option for a test (requires multiple assemblies), unless we ask to test out this PR from all users @goerch @pierce-martin @williamrubens @inna-w @Cronan @barrybriggs @zhukovgreen @AlexCatarino @bobred who reported this in the bug tracker: |
Codecov Report
@@ Coverage Diff @@
## master #723 +/- ##
==========================================
+ Coverage 77.12% 77.14% +0.01%
==========================================
Files 62 62
Lines 5583 5583
Branches 891 892 +1
==========================================
+ Hits 4306 4307 +1
+ Misses 985 980 -5
- Partials 292 296 +4
Continue to review full report at Codecov.
|
|
In that case, I'd lean towards merging this as is, if you don't object. |
|
@filmor i think this PR still misses this essential part about handling exceptions from "transitive" dependencies, which I experienced before (I'm going to find a link), see this comment from @Cronan: https://github.com/pythonnet/pythonnet/pull/528/files#r167013554 |
| @@ -461,5 +458,13 @@ public static Type LookupType(string qname) | |||
| } | |||
| return null; | |||
| } | |||
|
|
|||
| internal static Type[] GetTypes(Assembly a) | |||
There was a problem hiding this comment.
need a try-catch block here for transitive depedencies
There was a problem hiding this comment.
Isn't it an error, though, if we can't load transitive dependencies?
There was a problem hiding this comment.
@filmor i think it is someone else's job to load transitive dependencies, e.g. the direct dependencies should find them on their own:
|
@filmor i agree with your comment about skipassemblyload: #528 (comment) |
Codecov Report
@@ Coverage Diff @@
## master #723 +/- ##
==========================================
+ Coverage 68.55% 76.87% +8.32%
==========================================
Files 1 63 +62
Lines 283 5764 +5481
Branches 0 911 +911
==========================================
+ Hits 194 4431 +4237
- Misses 89 1029 +940
- Partials 0 304 +304
Continue to review full report at Codecov.
|
|
@denfromufa Can you please provide me with a test-case for the transitive dependency issue you mention? I have really no idea what this is about. |
| // Return all types that were successfully loaded | ||
| return exc.Types.Where(x => x != null).ToArray(); | ||
| } | ||
| } |
There was a problem hiding this comment.
@denfromufa Is this what you had in mind?
There was a problem hiding this comment.
@filmor this is even more than what i asked for :)
This is basically what @dmitriyse prepared in PR 528 without the event code that I don't understand.
What does this implement/fix? Explain your changes.
Useless calls to load dependencies based on private and nested types.
Does this close any currently open issues?
#703
Any other comments?
...
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG