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

Feature/per directory coverage #109

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dothebart
Copy link

@dothebart dothebart commented Dec 3, 2020

as discussed in #108 I need another formatter that groups sources by directory and other criteria.

I would make this a draft pr since this is still very hacky, but for some reason github won't let me. [edit it created a draft. hm.]

the yml for grouping looks like this:

Cache:
  coverage/arangod/Cache
Cluster:
  coverage/arangod/Cluster
  coverage/arangod/Network
  coverage/arangod/Sharding

Tabular per directory output achieved by this (using the regular return):

Directories                                         Stmts    Miss    Cover  Missing
------------------------------------------------  -------  ------  -------  ---------
coverage/arangod/Cluster                            14254    3752  73.6776
coverage/arangod/ClusterEngine                        889     275  69.0664
coverage/arangod/Network                              689     169  75.4717

Wild print of per topic collected percentages:

Cluster - +76.04% %

@aconrad
Copy link
Owner

aconrad commented Dec 3, 2020

Hi @dothebart, thanks for your pull request!

I took a quick glance at your changes and I think I better understand what you are trying to do; although having a screenshot of the output in the PR description would definitely help. I also acknowledge that you said that it was still a draft.

From my point of view, I worry that this new reporter is a little too specific to your use case and wonder if it should be part of pycobertura. I tried to design pycobertura in a way that allows extensibility by creating custom reporter like you did. I don't have a lot of experience extending the CLI portion of pycobertura, but wouldn't it be possible for you to keep the implementation of a new reporter in its own repository? Say pycobertura-directory-reporter-plugin and then use that to extend the behavior of pycobertura?

I would be happy to take changes to pycobertura to accommodate its extensibility, however. Again, without much experience extending pycobertura myself, there's likely room to make it easier on users. For example, I could see us introduce a way to register new formatters for the command line. Formatters are currently registered in a dictionary {"<reporter_name>": <reporter class>} (like you did in this PR) and there might be better ways to modify that object without monkey patching a global variable.

What do you think?

@dothebart
Copy link
Author

dothebart commented Dec 3, 2020

Added the sample output.

In general you see that it sort of plugs nicely into the reporters interface, so having a CLI-Reporter plugin would probably exactly fit my needs. It would probably then be 2 different formatters.

The code here still all a bit rough around the corners, since I was a bit under time pressure, and working with the 20Megs xml took several minutes on each try ;-) So I didn't add tests or such yet.

So I thought we can use this as a discussion start rather than theoretically talking about what could or should be.
I found nice about the CLI that I didn't have to do much to get my things done, it mostly doesn't even interfere
with the rest of the code aside of deriving a readily existing base class.

@aconrad
Copy link
Owner

aconrad commented Dec 3, 2020

Absolutely! Totally open for discussion. I'm sharing preliminary thoughts about what crossed my mind at a first glance.

And if you see performance improvements along the way since you are dealing with a large XML file, we welcome change here, too! 😉

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