feat: GitHub and JSON formatted printers #134
Conversation
There was a problem hiding this comment.
Hi @estahn
Thanks a lot for opening the PR. This is a big step towards a very useful feature. I did not yet do a full review, but left some comments which I spotted on a quick glimpse.
In addition, we would need the following before merging:
- Tests of the new functionality
- Type-Hints, where possible
- Docstrings ;-)
| raise NotImplementedError() | ||
|
|
||
|
|
||
| class LegacyPrinter(Printer): | ||
| """Printing functionality consistent with the original early-versions docstr-coverage outputs. | ||
|
|
||
| In future versions, the interface of this class will be refined and an abstract superclass |
There was a problem hiding this comment.
I guess we can remove this comment in the docstring
|
|
||
| success = total_results["coverage"] >= self.fail_under | ||
|
|
||
| print_line("## [:books: docstr_coverage](https://docstr-coverage.readthedocs.io/en/latest/api_essentials.html) status: %s **%s**" % (self._status_icon(success), self._status_text(success))) |
There was a problem hiding this comment.
We should probably link to the github readme instead of the RTD (which should be significantly improved or disabled anyways)
| @@ -147,3 +164,63 @@ def _print_overall_statistics(self, results): | |||
| ) | |||
|
|
|||
| print_line("Total coverage: {:.1f}% - Grade: {}".format(count.coverage(), grade)) | |||
|
|
|||
|
|
|||
| class JsonPrinter(Printer): | |||
There was a problem hiding this comment.
I assume that the JsonPrinter is primarily used in use cases where the output is interpreted by a machine (which thus allows easy deserialization). We've had such feature requests in the past.
Thus, the printer should provide the maximum amount of available information. The results.to_legacy() only provides an aggregated overview; it would be better to serialize all information present in results (which, as opposed to results.to_legacy(), may also be extended with additional fields in the future).
If that's too much work, we can of course remove the JsonPrinter in this PR and open a dedicated issue/PR for that.
| print(json.dumps({"files": file_results, "result": total_results})) | ||
|
|
||
|
|
||
| class GitHubCommentPrinter(Printer): |
There was a problem hiding this comment.
I am understanding it correctly that this is indeed Github specific (and not plain markdown) because of the icons you use, right? Just asking to double-check to understand why the class is named as it is. Maybe we should call it GithubMarkdownPrinter?
In either case, some docstring could not hurt ;-)
| @@ -5,6 +5,7 @@ | |||
| import enum | |||
| import functools | |||
| import operator | |||
| import os | |||
The issue with
print_linesusinglogger.infostill persists. The issue being thatdocstr_coverage > results.txtdoes contain the progress bar. I suggest either add a parameter for an output file or remove the progress bar.This is how it looks like:
📚 docstr_coverage status: ❌ FAILED
Overall coverage:
76%(required80%)Additional details and impacted files
docstr_coverage/__init__.pydocstr_coverage/badge.pydocstr_coverage/cli.pydocstr_coverage/config_file.pydocstr_coverage/coverage.pydocstr_coverage/ignore_config.pydocstr_coverage/printers.pydocstr_coverage/result_collection.pydocstr_coverage/visitor.py