search

HunterMcGushion / docstr_coverage

Docstring coverage analysis and rating for Python
MIT License
96 stars 19 forks source link

Add coverage badge #12

Closed fabiosangregorio closed 4 years ago

fabiosangregorio commented 4 years ago

Love the project! It works really well in discovering what docstrings are missing and how much the software is documented. Thank you for that.

I was wondering if an option to output a status badge could be added, or in alternative an option to only output the overall percentage score so we could create our Shields.io custom status badge with the documentation percentage coverage.

PS: sorry, I don't know what happened there. I might have accidentally submitted the issue while i was writing :)

HunterMcGushion commented 4 years ago

Thanks for the suggestion, @fabiosangregorio! I love the idea of adding a badge, unfortunately I’ve never done that so I’m not sure how to go about adding one right now. Would you be interested in looking into it/submitting a PR?

fabiosangregorio commented 4 years ago

I'm looking into it, although it looks a bit complicated. I'll keep you posted.

killthekitten commented 4 years ago

@fabiosangregorio great idea, and I didn't know about shields.io!

I would suggest to copy what https://github.com/dbrgn/coverage-badge does, it seems pretty lightweight, although it piggybacks the complex coverage.py output format. They also use shields.io by the way. It would require some boilerplate from the end user, of course.

fabiosangregorio commented 4 years ago

What I'm using

I'm currently using a custom python script I wrote to generate a shields.io static badge and write it in the README.md, and then committing the file through a Github Action.

It just:

Loads the docstr_coverage output file and extracts the total coverage percentage:

import re
from urllib.parse import quote

with open("./documentation-reports/docstr-coverage.txt", "r") as f:
    last_line = f.read().splitlines()[-1]

percentage = int(float(re.search("\\d+(?:\\.\\d+)?", last_line).group()))

Computes the color based on the percentage and creates the shields.io badge URL:

if percentage >= 90:
    color = "brightgreen"
elif percentage >= 80:
    color = "green"
elif percentage >= 60:
    color = "yellowgreen"
elif percentage >= 40:
    color = "yellow"
else:
    color = "red"

query = quote(f"docs-{percentage}%-{color}")
url = f"https://img.shields.io/badge/{query}"

And writes the URL in a specific part of README.md

with open("./README.md", "r+") as r:
    data = r.read()
    data = re.sub(
        r"docs-coverage\"(\W)*src=\"\S*\"", f'docs-coverage" src="{url}"', data
    )
    r.seek(0)
    r.write(data)
    r.truncate()

This is very custom to my setup though.

Why I'm using this

It looks like that to create a dynamic badge that gets updated with the current docstring coverage status you need to rely on an external service and get the badge accepted on the shields.io repository.

Badge implementation on the shields.io repo looks like a lot of work for something that should be really trivial.

What could be done

There could be an option on the docstr_coverage package to output the svg badge file, just like @killthekitten suggested. This would work well because in my use case I would only need to commit the new file instead of running this bodged script, linking the svg asset from my repo in the README.md.

I would also really just copy https://github.com/dbrgn/coverage-badge without the Coverage.py dependency.

HunterMcGushion commented 4 years ago

Wow, thanks for being so thorough, @fabiosangregorio! This is a really cool solution. My concern is that, as you already said, "This is very custom to [your] setup", and I think some people might have some reservations about letting us edit their readme directly. This is some seriously awesome work, though, and I applaud your creativity! Just so I can better understand what you're doing, does your process look something like this?

  1. Add some stub badge to your readme, so it can later be replaced
  2. Make a GitHub Action that runs docstr-coverage and pipes the output to a temporary file
  3. Update the readme's stub badge with the new percentage
  4. Clear out the temporary file
  5. ? Action pushes a new commit to the PR with updated readme (I'm unclear on this one)

I'm new to GitHub Actions, so I'm sure I got some of that wrong, but I really appreciate you outlining the process for us! Poking through your docs.yml in your Telereddit repo has been very educational!

I'd love something like what you and @killthekitten suggest: An additional CLI option to output a badge to a file, which can just be linked to in the readme. Unfortunately, (if I'm understanding your GitHub Action correctly) this would still require an additional commit to push the new badge file, which seems like it could get messy/be a bit annoying. I'm still trying to think of a way to update it live like Coveralls, Codacy, etc. without having to build a whole service and get GitHub permissions from users...

Again, thanks for putting in so much work on this! Also, I love your Telereddit repo! It's really clear that you put tons of effort into it!

killthekitten commented 4 years ago

@HunterMcGushion as I understood @fabiosangregorio, he suggests to implement a CLI command/option that outputs an updated SVG once the checks are finished. The integration is entirely up to the user then.

It could look like this:

docstr-coverage app/ -d .docstrignore --failunder 90 --generate-badge
fabiosangregorio commented 4 years ago

he suggests to implement a CLI command/option that outputs an updated SVG once the checks are finished. The integration is entirely up to the user then.

This is right @killthekitten, this would be totally independent from Github Actions. One could also run this command on a pre-commit hook and directly commit the file without running any pipeline.

  1. ? Action pushes a new commit to the PR with updated readme (I'm unclear on this one)

You really got the exact point of my solution and the problem that comes with it @HunterMcGushion ! The Github Action I'm using adds, commits and pushes changes made by the workflow to the repository:

- name: Commit changes
      uses: EndBug/add-and-commit@v4
      with:
        message: "Commit from Github Actions: docs workflow changes"
      env:
        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

Therefore, every commit I make that triggers the docs.yml workflow (and generates the badge URL) will have duplicate commits just for the badge modification, just like you can see in the following screenshot and diff:

<a href="https://codeclimate.com/github/fabiosangregorio/telereddit/maintainability"><img
src="https://api.codeclimate.com/v1/badges/bef15455da0878eae539/maintainability" alt="Maintainability"></a>
- <img class="docs-coverage" src="https://img.shields.io/badge/docs%20coverage-70%25-yellowgreen" alt="Docs coverage">
+ <img class="docs-coverage" src="https://img.shields.io/badge/docs%20coverage-78%25-yellowgreen" alt="Docs coverage">
<a href="https://github.com/psf/black"><img src="https://img.shields.io/badge/code%20style-black-000000.svg"
alt="Code style: black"></a>

Unfortunately, (if I'm understanding your GitHub Action correctly) this would still require an additional commit to push the new badge file, which seems like it could get messy/be a bit annoying.

This is also right. Having the option for a file output would mean that:

I'm still trying to think of a way to update it live like Coveralls, Codacy, etc. without having to build a whole service and get GitHub permissions from users

This is what I was thinking about, and it's what dynamic badges approved in the shields.io repository actually do. Coveralls is a service and therefore it:

  1. gets triggered on the workflow build (by the coverage artifact upload)
  2. runs the coverage report, computes the coverage percentage
  3. ? triggers shields.io to create the badge (I'm unsure of this)
  4. The badge URL remains the same, but the SVG file hosted on shields.io changes. So no README edits are needed. (e.g. https://img.shields.io/coveralls/github/fabiosangregorio/telereddit)

TL;DR

Implementing the badge file output is a great way to start.

The only way that satisfies the requirement of not having to commit files through Github Actions is implementing a dynamic badge on the shields.io repository. As I said previously, this looks a bit complicated and overkill but it's the only way I see right now.

Following the official shields.io tutorial on creating a badge, you would need to open a PR on their repository with the new node.js badge implementation, but I'm pretty unclear on what the implementation would be. The learning curve on that process seems a bit steep.

PS: sorry for the long comment, but I needed this to clear our doubts and making sure this is what is actually happening :)

PPS: Thank you for the love on my repository! I'm using it to deepen the skills and understanding of a structured, robust and tested python project, and your package is helping with it :)

HunterMcGushion commented 4 years ago

he suggests to implement a CLI command/option that outputs an updated SVG once the checks are finished. The integration is entirely up to the user then.

This is right @killthekitten, this would be totally independent from Github Actions. One could also run this command on a pre-commit hook and directly commit the file without running any pipeline.

Thank you both for clarifying!

Implementing the badge file output is a great way to start.

This sounds great! Are you still interested in opening a PR to implement this?

PS: sorry for the long comment, but I needed this to clear our doubts and making sure this is what is actually happening :)

Haha no apology necessary! I'd much rather be completely clear and all on the same page. Thank you for taking the time to walk me through everything!

fabiosangregorio commented 4 years ago

Sadly in these months I'm extremely busy with university exams so I won't be able to :(

Let me know if you have any other ideas to implement the whole badge-with-no-commit solution though!

HunterMcGushion commented 4 years ago

Understood! Thank you for your help, and good luck with your exams!

HunterMcGushion commented 4 years ago

Hey, @fabiosangregorio and @killthekitten! When you get a chance, could you take a look at #22, please? You should be able to install it with

pip install git+https://github.com/HunterMcGushion/docstr_coverage.git@feat/badge

I'd like to add a few more tests, but it is all working now. Is this what you two were picturing? If you have time to do a formal review and make some suggestions, I'd very much appreciate it!

fabiosangregorio commented 4 years ago

This is exactly what I was thinking about! I reviewed the PR, very well implemented. I'm trying it out and it looks like it does the job. I don't need to run my custom script anymore, I just have to commit the new badge image, as expected.

I just want to point out two things:

HunterMcGushion commented 4 years ago

@fabiosangregorio, thanks for taking a look!

should the badge get created/overwritten only if the process has exited successfully?

Hmmm that's a very good point... I suppose it's safe to assume that if someone is using --failunder and it does fail, then they're going to want to fix the problem before merging anything, so the badge is useless. If they do decide to merge despite the failure, though, I think the badge should reflect the actual coverage. I'm feeling a bit conflicted about this. Do you feel strongly that on failure, the badge shouldn't be created?

Let me know if you want me to open a new issue for this last one.

That would be great! Definitely want to sort out that weird bug. Thanks a lot!

fabiosangregorio commented 4 years ago

I think the badge should reflect the actual coverage

Yeah, thinking more about it, I definitely agree. I think this setup is great and is the best that can be done without using an active service to serve the badges.

HunterMcGushion commented 4 years ago

@killthekitten, sorry if I merged before you had a chance to review the PR, but if you have any problems/concerns, we can absolutely reopen this issue or open a new one, as I'd love your feedback!

killthekitten commented 4 years ago

@HunterMcGushion sorry for taking too long, I'll definitely take a look at it sooner or later.