- Notifications
You must be signed in to change notification settings - Fork750
Use GetExportedTypes where possible and filter nested types#723
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This is basically what@dmitriyse prepared in PR 528 without the eventcode that I don't understand.
@denfromufa Any idea for a test? |
den-run-ai commentedAug 24, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
need a try-catch block here for transitive depedencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Isn't it an error, though, if we can't load transitive dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@filmor i think it is someone else's job to load transitive dependencies, e.g. the direct dependencies should find them on their own:
den-run-ai commentedAug 24, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@filmor i agree with your comment about skipassemblyload:#528 (comment) |
codecovbot commentedAug 27, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@denfromufa Is this what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@filmor this is even more than what i asked for :)
Uh oh!
There was an error while loading.Please reload this page.
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