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

Inject basedir - allow injecting IAppDomain #1906

Merged
merged 5 commits into from
Jan 14, 2017
Merged

Conversation

304NotModified
Copy link
Member

@304NotModified 304NotModified commented Jan 13, 2017

  • having in two classes (LogManager & LogFactory) the (static) currentAppDomain is confusing
    • Moved logic to LogFactory,
    • removed internal stuff in LogManager
    • same logic will be executed
  • cleanup of event-hook so we don't double hook
  • don't use AppDomainWrapper everywhere, but the static IAppDomain property
  • remove old #if stuff and handle empty properties of IAppDomain

Goal: inject basedir in another package, by using the IAppDomain


This change is Reviewable

As the interface is altered for SL, this could a breaking change for SL. But i'm pretty sure there aren't custom implementations of IAppDomain for SL in custom code.

@304NotModified 304NotModified force-pushed the inject-basedir branch 2 times, most recently from 2b0804f to 3fda31c Compare January 13, 2017 20:39
@304NotModified 304NotModified added the enhancement Improvement on existing feature label Jan 13, 2017
@codecov-io
Copy link

codecov-io commented Jan 13, 2017

Current coverage is 81% (diff: 56%)

Merging #1906 into master will decrease coverage by <1%

@@             master      #1906   diff @@
==========================================
  Files           285        285          
  Lines         19398      19409    +11   
  Methods        2946       2945     -1   
  Messages          0          0          
  Branches       2256       2260     +4   
==========================================
- Hits          15803      15795     -8   
+ Misses         3058       3056     -2   
- Partials        537        558    +21   

Sunburst

Powered by Codecov. Last update c65edc2...d4786d5

@snakefoot
Copy link
Contributor

@304NotModified Looks good to me. Like the removal of the if-defined.

Since LogManager is pretty much a static interface to the LogFactory, then it makes sense there is no "logic" in LogManager.

@304NotModified 304NotModified merged commit 3e50a4c into master Jan 14, 2017
@304NotModified 304NotModified changed the title Inject basedir - allow one current IAppDomain, open IAppDomain in all platforms Inject basedir - allow injecting IAppDomain Jan 14, 2017
@304NotModified 304NotModified deleted the inject-basedir branch February 5, 2017 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement on existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants