-
Notifications
You must be signed in to change notification settings - Fork 204
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
[ENH, MRG] Make orphan of unused API entries #983
Conversation
Hmmm the Circle failure is quite odd and the test failure was because the backreferences file was a directory which I didn't know was allowed... would anyone have a minute to review this? |
Sphinx does not give the nicest tracebacks. Have you tried
Have you looked at the structure it produces locally? My guess is that the backreferences files end up somewhere where some other files do (like |
Thanks @larsoner, that was really helpful, I think I figured it out! |
I think this turned out really well! You have to zoom in a bit since there are a lot of entries in some cases but I think super useful. Unused API Entries — Python documentation.pdf I'm not sure I handled |
From a quick look, code looks reasonable! But CIs are unhappy. Let me know when things are green or you need help interpreting To demo this functionality, could you open a PR to MNE-Python demoing this functionality? You can tell CircleCI over there to use your branch in https://github.com/mne-tools/mne-python/blob/64dff6189215cbead61db81d6e6de811a05633e3/tools/circleci_dependencies.sh#L15 |
Hmmm, I'm getting this and similar warnings causing Circle to fail |
|
Ah yeah that makes sense, it's warning for |
You could try |
Ok now this seems like a bug with |
Ok there's two things I'm a bit stuck on: 1) The bug in the name resolving code (above) for |
Well I realized that modules should be lower case AND share a root with functions or classes in that module whereas classes should be camel case (and also share a root with their methods) so that fixes that. I'm not sure if the |
Since the while section was made optional, I got rid of the |
Ok, this is good to merge by me |
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.
Thanks this is a great addition, quick review and mostly some doc nitpicks. I have not been following this closely so please excuse any comments that are not relevant/don't make sense.
doc/configuration.rst
Outdated
will make one graph per module with all of the API entries connected to | ||
the example that they are used in. This could be helpful for making a map | ||
of where to look in a project if you want to learn about a particular | ||
module. |
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 add that the graphviz package is required if you want the graphs?
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.
Also maybe adding an image of an example graph (e.g., of a sphinx gallery module) would be nice?
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.
I added the link to the sg_api_usage.html
in the sphinx_gallery
documentation, I think that might be a good way to show what it looks like!
Thanks, I think there were some great clarifications! |
The artifact looks good https://output.circle-artifacts.com/output/job/b2109ab5-5b22-4fa9-b0fc-30657ae11c14/artifacts/0/rtd_html/configuration.html, this is good to go by me! |
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 +1 for merge, feel free to merge when you're happy @lucyleeow !
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.
Last thing, sorry!
Co-authored-by: Lucy Liu <jliu176@gmail.com>
Thanks @alexrockhill and @larsoner ! |
Fixes #974.
I just made a bulleted list of unused API entries and the percentage used so far and then a bulleted with sub-bullets for used API entries and where they are used. As discussed with @larsoner and @drammock, it would be nice to make them into graphs with
graphviz
but we would also have to handle lacking this dependency gracefully which I'm not as sure how to do. Pretty good start I think though!Unused API Entries — Python documentation.pdf