-
Notifications
You must be signed in to change notification settings - Fork 45
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
Improve exported plugin docstrings and annotations #725
base: main
Are you sure you want to change the base?
Conversation
Thanks for the improvements. Looks really good! We will discuss it with the team and come back to you. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #725 +/- ##
==========================================
+ Coverage 76.65% 76.71% +0.06%
==========================================
Files 316 315 -1
Lines 27101 27118 +17
==========================================
+ Hits 20774 20804 +30
+ Misses 6327 6314 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
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.
Nice cleanup, looks good, just some small things.
Thanks for the quick review! We've applied all suggestions where possible in b1e5a03. |
b2d8ad3
to
ccd9cf0
Compare
Thanks for the additional review! It seems like we are still left with the following
Once these are fixed (or ignored?) we can enable the |
Did 13cf9fb fix that? So can |
58aa4ad
to
ee2509b
Compare
Unfortunately not. It seems like these warnings can be caused by multiple things: readthedocs/sphinx-autoapi#180 |
39d214d
to
a34a66a
Compare
Using the following modified sphinx build options we are able to dive into this issue a little deeper:
It seems that these
The class |
I don't really understand how it works either 😄 @Miauwkeru can you comment on the above regarding the |
the |
Thanks for clarifying @Miauwkeru. We have removed the file in c34a00a. |
Could this PR be looked at soon? I am not looking forward to keep updating and fixing the issues the new tests find in new functions. The sooner this is in the main test suite the better :) |
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, @pyrco?
TypeError: Parameters to generic types must be types.
Co-authored-by: Erik Schamper <1254028+Schamper@users.noreply.github.com>
Co-authored-by: Erik Schamper <1254028+Schamper@users.noreply.github.com>
Co-authored-by: Erik Schamper <1254028+Schamper@users.noreply.github.com>
b7877bd
to
8be39b4
Compare
This PR improves uniformity across all exported plugin functions. A new test in
tests/test_plugin.py
(b3539e1) runs the following checks when iterating overfind_plugin_functions
.Plugin methods should:
Plugin classes are also checked for valid rst docstrings.
Further more we think the
tox -e docs-build
command should be extended with the-W
argument, making doc builds fail when a warning is generated by Sphinx. We have suppressed certain warnings as can be seen intests/_docs/conf.py
, which should make the build warning log more clear. We can add this argument as soon as a PR insphinx-argparse-cli
is merged (tox-dev/sphinx-argparse-cli#171).Please let us know what you think of this improvement. This will certainly help our internal build and development process as it forces plugin developers to be more concise when defining export functions.