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

ci: add coverage check #50

Merged
merged 1 commit into from
Jul 8, 2024
Merged

ci: add coverage check #50

merged 1 commit into from
Jul 8, 2024

Conversation

mikix
Copy link
Contributor

@mikix mikix commented Jul 5, 2024

Deletes some unused code, adds a few tests, and gets us to 100%.

Checklist

  • Consider if documentation (like in docs/) needs to be updated
  • Consider if tests should be added

Copy link

github-actions bot commented Jul 5, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
693 693 100% 99% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
chart_review/agree.py 100% 🟢
chart_review/cli.py 100% 🟢
chart_review/cohort.py 100% 🟢
chart_review/commands/accuracy.py 100% 🟢
chart_review/common.py 100% 🟢
chart_review/external.py 100% 🟢
TOTAL 100% 🟢

updated for commit: a04be98 by action🐍

@mikix mikix force-pushed the mikix/coverage branch 4 times, most recently from 89115ee to 3476e83 Compare July 5, 2024 20:04
@mikix mikix force-pushed the mikix/coverage branch from 3476e83 to a04be98 Compare July 5, 2024 20:16
@@ -62,30 +62,11 @@ def confusion_matrix(
elif not truth_positive and not annotator_positive:
TN.append(key)
else:
raise Exception("Guard: Impossible comparison of reviewers")
raise Exception("Guard: Impossible comparison of reviewers") # pragma: no cover

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i sort of think this could be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little torn on that. I kind of agree with you - but I also think it's kind of nice to have a catch-all at end in case of future human error.

🤷 Arguably that's what tests are for...

But if I remove this, I'm also tempted to change the last elif to an else and losing that symmetry / self-documentation of the checks would be bad. So I'd probably keep it as a dangling elif which makes my hairs stand up a bit.

I dunno - I could be convinced since it is a pointless line. But maybe I need to sleep on it.

@mikix mikix merged commit db5928e into main Jul 8, 2024
2 checks passed
@mikix mikix deleted the mikix/coverage branch July 8, 2024 13:13
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