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

add docstr-coverage workflow #1409

Merged
merged 4 commits into from
Jan 27, 2021
Merged

Conversation

epassaro
Copy link
Member

@epassaro epassaro commented Jan 9, 2021

Description

I used the docstr-coverage package to check the docstring coverage of TARDIS. This package is well maintained and offers an interesting number of options that can be declared on the .docstr.yaml file.

To try these changes locally, first install docstr-coverage latest commit:

pip install docstr-coverage

and then run docstr-coverage on tardis repository folder.


To integrate this package with our continuous integration cycle, I wrote a simple GitHub workflow which runs docstr-coverage twice:

  • First, on the base of the pull_request and set the coverage as future THRESHOLD value.
  • Then, on the HEAD of the pull_request, failing if coverage is lower than THRESHOLD (for example, after writing a new function, class or method without any docstring).

BEFORE MERGE:

  • Discuss the final options to be used in .docstring.yaml.
  • Remove the git checkout ${{ github.event.pull_request.head.sha }} -- .docstr.yaml line. The workflow will fail after doing this because currently .docstr.yaml does not exist on master, but this is fine.

Motivation and Context

We want to get some information about the status of TARDIS documentation coverage and integrate that on our CI cycle.

How Has This Been Tested?

  • Testing pipeline
  • Other (please describe)

New GitHub Action.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • None of the above (please describe)

Checklist:

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have built the documentation on my fork following these instructions
  • I have assigned and requested two reviewers for this pull request

@codecov
Copy link

codecov bot commented Jan 9, 2021

Codecov Report

Merging #1409 (1650c06) into master (4a6a8a6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1409   +/-   ##
=======================================
  Coverage   71.16%   71.16%           
=======================================
  Files          67       67           
  Lines        5521     5521           
=======================================
  Hits         3929     3929           
  Misses       1592     1592           

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 4a6a8a6...1650c06. Read the comment docs.

@epassaro epassaro force-pushed the docstr-coverage branch 3 times, most recently from 06313e2 to 9de1222 Compare January 10, 2021 01:31
.docstr.yaml Outdated Show resolved Hide resolved
minor changes

run on commited files only

run on commited files only (2)

run on commited files only (3)

run on commited files only (4)

run on commited files only (5) [skip ci]

fail if coverage is lower than previous one

fix typo

make job conditional

make job conditional (2)

test update job

test update job (2) [skip-ci]

test update job (3) [skip-ci]

test update job (4) [skip-ci]

test update job (5) [skip-ci]

test update job (6) [skip-ci]

test update job (7) [skip-ci]

round value

store commit sha [skip-ci]

store commit sha (2) [skip-ci]

try simpler approach [skip-ci]

try simpler approach (2) [skip-ci]

try simpler approach (3) [skip-ci]

try simpler approach (4) [skip-ci]

try simpler approach (5) [skip-ci]

try simpler approach (6) [skip-ci]

use docstr-coverage latest release
@andrewfullard andrewfullard merged commit 72fd51c into tardis-sn:master Jan 27, 2021
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
* add docstr-coverage workflow

minor changes

run on commited files only

run on commited files only (2)

run on commited files only (3)

run on commited files only (4)

run on commited files only (5) [skip ci]

fail if coverage is lower than previous one

fix typo

make job conditional

make job conditional (2)

test update job

test update job (2) [skip-ci]

test update job (3) [skip-ci]

test update job (4) [skip-ci]

test update job (5) [skip-ci]

test update job (6) [skip-ci]

test update job (7) [skip-ci]

round value

store commit sha [skip-ci]

store commit sha (2) [skip-ci]

try simpler approach [skip-ci]

try simpler approach (2) [skip-ci]

try simpler approach (3) [skip-ci]

try simpler approach (4) [skip-ci]

try simpler approach (5) [skip-ci]

try simpler approach (6) [skip-ci]

use docstr-coverage latest release

* update .docstr.yaml

* remove line (workflow will fail)

* blame files on pr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants