-
Notifications
You must be signed in to change notification settings - Fork 78
Doc err macros #2260
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
Doc err macros #2260
Conversation
|
I've removed some flags here, which were unused. I guess this is technically a breaking change. |
Codecov Report
@@ Coverage Diff @@
## main #2260 +/- ##
==========================================
- Coverage 93.29% 93.29% -0.01%
==========================================
Files 27 27
Lines 26076 26073 -3
Branches 1167 1167
==========================================
- Hits 24328 24325 -3
Misses 1718 1718
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
jeromekelleher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thanks so much for doing this @benjeffery 🤩
This is great.
c/tskit/core.h
Outdated
| */ | ||
| #define TSK_ERR_BAD_OFFSET -200 | ||
| #define TSK_ERR_OUT_OF_BOUNDS -201 | ||
| /* 201 was the now unused TSK_OUT_OF_BOUNDS */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might as well use 201, maybe move TSK_ERR_SEEK_OUT_OF_BOUNDS up here and change its code to 201? That's the least noisy change, and it's vanishingly unlikely anyone is depending on its specific value.
python/requirements/development.txt
Outdated
| pytest-cov | ||
| pytest-xdist | ||
| PyVCF | ||
| #PyVCF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is accidental I assume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cruft from when I was attempting Py3.11.
|
@jeromekelleher As I'm finishing this up I realise that some of the errors I am documenting are only thrown by undocumented parts of C (although they maybe documented in Python) This is quite fiddly to check for each one, so I am defaulting to documenting them. The only issue I can see with this is if someone decides to use the error macros, which seems unlikely and not a great thing for someone to do anyway. |
|
Agreed |
db206b1 to
d0c5243
Compare
|
@jeromekelleher This is ready to go, did another pass through and found a couple of tweaks (e.g. max alleles is not 127 anymore!) |
c1b0425 to
4e6c287
Compare
jeromekelleher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I wonder if we should leave some of the groups out from the final docs, but it's up to you.
| .. doxygengroup:: DISTANCE_ERROR_GROUP | ||
| :content-only: | ||
|
|
||
| ------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should leave HAPLOTYPE_ERROR_GROUP, DISTANCE_ERROR_GROUP, IBD_ERROR_GROUP, MAPPING_ERROR_GROUP, and STATS_ERROR_GROUP out for now? Seems odd to put these in to the end-user documentation when we're not providing the corresponding functions in the API. You've done the hard work, to adding the groups back in to the RST here will be very easy later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, got a bit carried away. Will fix in the in-progress polish PR.
WIP!