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

Logging concept #106

Closed
wants to merge 12 commits into from
Closed

Logging concept #106

wants to merge 12 commits into from

Conversation

coenm
Copy link
Contributor

@coenm coenm commented May 21, 2018

Since version 2.0 I have an issue with a project running on Linux where coverage is not calculated (#72 and #107) . Running on Windows doesn't have any problems. My problem is that it is pretty hard to do investigation on Travis and why my project isnt covered.

Would it be nice if you can enable logging in order to give the user the ability to do a first investigation (and maybe fix and send a PR) before adding an issue.

First of all, I dislike lines like logger.Log("bla bla"); through all production code at all kind of places. IMO too much of these lines blur the original intention of the code.

This PR is a starting point of how logging can be inplemented. Hopefully, you all see the value of logging and we can have a discussion about this.

The logging is done by decorating classes with a LoggingDecorator.

  1. https://github.com/coenm/coverlet/commit/f28a7047d2800026c2f8e4ab41e97c16a44a0c04 Added ILogger, ConsoleLogger, and LoggerFactory. No functionality changed.
  2. https://github.com/coenm/coverlet/commit/0888e0d7bb3ca48c14dcd0d8ddcd264ddb223e63 Added new interface to existing Instrumenter class. No functionality changed.
  3. https://github.com/coenm/coverlet/commit/3cd15cecea760cc7fa93cb18178879fc468e19e9 Creation of Instrumenter is done using static Factory class. No functionality changed.
  4. https://github.com/coenm/coverlet/commit/2010ed942504b01211e73fc20eab1ed87ac5095b Added InstrumenterLoggerDecorator to decorate an instance of IInstrumenter. No functionality changed.
  5. https://github.com/coenm/coverlet/commit/6f2fc30a012e6be44d923aaf9439f416b12c2ce8 Added functionality to register the decorator to the InstrumenterFactory. EnableLogging class has responsibility to create and add the logging decorator to the InstrumenterFactory. No functionality changed.
  6. https://github.com/coenm/coverlet/commit/80aef4b6ee7537cbc95990a072aa30ef24343f1d This step adds a property Verbose to the InstrumentationTask and when this property is true, it enables Logging.
  7. https://github.com/coenm/coverlet/commit/b4e66f7adc6970a08ea1e0351f9a634364c2ee3a Seperated classes to own files.

Please note that the original code was extended (decorated) and hardly touched. Only the creation of Instrumenter is now done using a factory, and the Instrumenter class now implements an interface.

If you like this idea, I'm willing to take this further

BTW, I don't expect this PR to be accepted as is right now as it doesn't add real value at this point.
I just didn't want to put too much effort in this without knowing what the you think about logging and about this concept.

coenm added 9 commits May 21, 2018 21:05
…(without decorating the instance with a logger or something like that). The creation of Instrumenter in Coverage class is done using the static Instrumenter factory. Still no functionality has changed
…ll registered decorators. Added EnableLogging method that registers the logging decorator to the InstrumenterFactory
@codecov
Copy link

codecov bot commented May 21, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@74fa3a2). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #106   +/-   ##
=========================================
  Coverage          ?   98.93%           
=========================================
  Files             ?       20           
  Lines             ?     1413           
  Branches          ?        0           
=========================================
  Hits              ?     1398           
  Misses            ?       15           
  Partials          ?        0
Impacted Files Coverage Δ
src/coverlet.core/Logging/EnableLogging.cs 100% <100%> (ø)
...verlet.core/Instrumentation/InstrumenterFactory.cs 100% <100%> (ø)
src/coverlet.core/Logging/ConsoleLogger.cs 100% <100%> (ø)
src/coverlet.core/Coverage.cs 100% <100%> (ø)
.../Logging/Decorators/InstrumenterLoggerDecorator.cs 100% <100%> (ø)
src/coverlet.core/Instrumentation/Instrumenter.cs 100% <100%> (ø)
src/coverlet.core/Logging/LoggerFactory.cs 100% <100%> (ø)

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 74fa3a2...b221bd5. Read the comment docs.

@coenm coenm changed the title Feature/logging Logging concept May 21, 2018
@coenm
Copy link
Contributor Author

coenm commented May 24, 2018

@tonerdo Are you interested in a logging feature. Would probably make investigating bugs like #107 easier.

@tonerdo
Copy link
Collaborator

tonerdo commented May 24, 2018

@coenm will take a look at this

@coenm coenm closed this Mar 11, 2019
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