Adding Markdown printer

[skv0zsneg]

Hello!

In case of issue #133 and not answered question #133 (comment) and not active PR #134 I took the initiative to implement markdown printer and refactor printers.

I recreated LegacyPrinter and added MarkdownPrinter by adding base class Printer in printers.py. Rendered Markdown output look like this:

While developing, I got some questions:

• Print to output is implemented with logging. I guess it needs tests in test_coverage.py. In my realization, I use data structures created with dataclasses maybe tests can be refactored by using them, and we'll could replace logging with print?
• I think creating output to stdout in markdown style doesn't increase readable in terminal. What if the new -o flag saves coverage stat results in the appropriate file with standard output in the terminal?

I don't write all unit tests beacuse of questions above. I hope these changes will help the project.

[MiWeiss]

Hi @skv0zsneg

Amazing. Thanks a lot! This is a contribution I (and I guess many others) have been waiting for forever.

The bad news are: I won't be able to do a thorough review in the next couple of weeks, but things should clear up mid october. Please do not hesitate to ping me if I have not reviewed the PR by then. Is this ok with you?

Print to output is implemented with logging. I guess it needs tests in test_coverage.py. In my realization, I use data structures created with dataclasses maybe tests can be refactored by using them, and we'll could replace logging with print?

This is a surprisingly sensitive topic 😄. Some of our users desperately want print, others want logging to better control the output location (although: yes, I am aware we're still mostly using root logger). I am thus I but unsure what to decide here. I am more and more tending towards having two parameters, one defining the format (text, markdown, ...) of the output, and the other one the location (path on fs, print, logging, ...). Maybe that's a bit much though for now - if you could do that as part of this PR, I'd be happy with having only print and file supported, and leaving logging for a future PR..

@HunterMcGushion any thoughts?

I think creating output to stdout in markdown style doesn't increase readable in terminal. What if the new -o flag saves coverage stat results in the appropriate file with standard output in the terminal?

Agreed. My suggestion above would fix that, though...

Again, thanks for opening the PR!

[skv0zsneg]

Thank you for answering!

Alright, I'll tag you here in the middle of October 🫡

About my questions - OK then, I'll add format defining (text, mardown) and location (print, path for file). If @HunterMcGushion will said to rewrite - I'll do it with no problem. Also will try to fix failed checks.

[HunterMcGushion]

@skv0zsneg, this looks fantastic! Thank you for putting all this together. I've got a couple minor comments that I'll make as I do my full review, but overall I love it! Thank you, @MiWeiss, for the quick response. I believe we can provide different formatters to logging depending on the target (in our case, stdout for a plain-text report vs a file for the markdown-formatted report). I might be thinking of a different logging library I've been using recently, though, so I'll do some digging. Personally, I prefer having all our logging done with the logging library and I feel like adding support for print might just confuse things. @MiWeiss, can you remind me when someone has specifically wanted to use print instead of logging? It seems to me that logging is preferable in most ways -- especially for a library that other projects may have as a dependency (such as docstr-coverage) since the end user has more control over which logs to silence.

In any case, thank you both! And again, amazing work @skv0zsneg! Please expect some review comments over the coming days as I have time.

[skv0zsneg]

@HunterMcGushion thank you for answering!
In order of your comments to the code and our discussions about logging using, I'll make some changes to this PR. Please do not review this version until I finish. I'll tag you!

I'm glad for helping this project <3

[skv0zsneg]

@HunterMcGushion @MiWeiss
I finished some refactoring work due to your comments above. The code is now ready for your judgment 😃

[MiWeiss]

Hi @skv0zsneg

Thanks so much, and sorry for the delay. I started my review. Didn't get to finish it completely yet, and still want to test everything, but I still thought I'd leave you some comments.

(* i do not expect many more comments from myself on that though, I'm quite excited for the feature. However, with @HunterMcGushion now also reviewing I'm only a sidekick-reviewer anyways :-D).

---

@HunterMcGushion:

@MiWeiss, can you remind me when someone has specifically wanted to use print instead of logging?

Hmmm... Well, no. I believe it had something to do with the standard formatting of logs and lazyness (aka convenience) of not having to change logging options (which is kind-of annoying when using a cli :-) ). Like, in some settings, INFO logs are suppressed by default. However, now that you're asking, I am not sure anymore, though (might have been my imagination, might have happend in a different library :-) ). Feel free to make your choice! If we ever wanted to change it, it doesn't have to be in this PR, imo.

[HunterMcGushion]

This isn't really the "format" of the output (that comes from the --format option. Maybe output target/location/destination. I can't really think of the right word. But this option is used to specify "where" to save the output rather than how to format the output

[skv0zsneg]

You right. I think the destination with combination of choices (stdout, file) clearly saying about param appointment.

Maby change --output to --destination?

[HunterMcGushion]

Yeah, that's a good idea. I like --destination

[HunterMcGushion]

@skv0zsneg, @MiWeiss, do we really need three different printer classes when the inheritance is as straightforward as it is here? It's just Printer > LegacyPrinter > MarkdownPrinter. Can Printer and LegacyPrinter be combined to make things a little simpler/flatter? This is an awesome feature, but I'm finding the code a bit difficult to navigate

[skv0zsneg]

@HunterMcGushion, sorry for delay!

The main reason for this type of class inheritance is the possible future features. For example, the project will need a web server (for some reason, idk), and it needs to have tools for rendering HTML results. So, you can just inherit from the printer and, using simple data structures, get a rendered HTML page.

If you think that this reason is weak, I can change inheritance.

[HunterMcGushion]

Thanks again, @skv0zsneg! We may need to do some refactoring down the road, but I'll approve/merge once this change is made.