Skip to content
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

fixes #63 - option to control assembly source #136

Merged
merged 4 commits into from
Oct 5, 2018
Merged

fixes #63 - option to control assembly source #136

merged 4 commits into from
Oct 5, 2018

Conversation

MV10
Copy link
Contributor

@MV10 MV10 commented Sep 29, 2018

When the DependencyContext parameter is null, the onNullDependencyContext defines the interpretation -- either to scan in-memory assemblies (the default, when available) or scan DLL files in the working directory (optional, or the fallback if the default is not available). See #63 for discussion.

@MV10
Copy link
Contributor Author

MV10 commented Sep 29, 2018

Tests are failing because xunit 2.x runs each test class in parallel and they're stepping on each other's changes to all the static members (either in the config package itself or all the test-support code, I'm not really sure -- but probably in DummyConsoleSink; could be the package's static reference to config though).

xUnit didn't fix their settings that disable parallelization in versions that support the older .NET Frameworks, so the quick fix is to just move the one new test into the same class where there was a collision because xUnit doesn't run methods within a class in parallel.

Burning a weekend afternoon nursing old unit tests for the sake of having passing tests is not the unit-testing happy-path...

@nblumhardt
Copy link
Member

I like the usability of this one, but it seems like a grey area - do we really want to get into assembly scanning/loading here? It's historically been a very delicate and flaky part of .NET to get involved in. I'm not totally against it, but I'm also a bit nervous about supporting/debugging the failures that will inevitably bubble back to us, here....

Thoughts?

@nblumhardt
Copy link
Member

Hmm, just looking again, I might have misunderstood how this is implemented - I see it's just switching between DependencyContext.Default and null. Took a moment to get up to speed via the other thread :-)

@nblumhardt
Copy link
Member

Just one nitpick comment, otherwise seems good-to-go, if we're satisfied this will sort out #63 👍

@MV10
Copy link
Contributor Author

MV10 commented Oct 5, 2018

No sweat, I debated about where to put that and whether to use a specific or general name.

Think this warrants documenting in the README? DependencyContext isn't explained so I figured both of these are sort of advanced usage, along the lines of "go read the code if you insist on playing with these"...

@nblumhardt
Copy link
Member

👍 I think we can deal with README updates down the line; pushing this through so we can assemble a final 3.0 dev build :-) 🎉

@nblumhardt nblumhardt merged commit 4db7753 into serilog:dev Oct 5, 2018
@MV10 MV10 deleted the issue_63 branch October 6, 2018 10:08
@merbla merbla mentioned this pull request Oct 7, 2018
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.

2 participants