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

fix docstring issues by adding an explicit handler #106

Merged
merged 3 commits into from Mar 30, 2022

Conversation

dwreeves
Copy link
Contributor

Long story short: fixes #104.

Locking the dependency uses mkdocstrings's so-called "experimental" handler for docstring parsing. The new one properly parses the docstring in this case whereas the legacy one does not.

@aminalaee
Copy link
Owner

aminalaee commented Mar 24, 2022

Nice find ! Do you have any references to this? Because I knew this issue was in mkdocstrings but I couldn't really find this info.

@aminalaee
Copy link
Owner

Unfortunately this one has some issues too, it adds some attributes automatically to the docs:
Screenshot 2022-03-24 at 09 53 01

@dwreeves
Copy link
Contributor Author

dwreeves commented Mar 24, 2022

@aminalaee This is a pretty new tool so I had to do my own digging to find that. I ran mkdocs build locally before and after having the experimental handler (python-mkdocstrings) in my environment

The experimental handler uses another tool under the hood called "griffe" which has a CLI; you can type "griffe sqladmin" to see how it parses the package.

@dwreeves
Copy link
Contributor Author

I think it's possible to not add those attributes; by default it always includes docstring'd methods, but I think you can override that. I'll check later today.

@aminalaee
Copy link
Owner

I'll open an issue on mkdocstrings and see how it goes, let's keep this open anyway. I think mkdocstrings is not wrong, we're making form_base_class = Form and it's rendering the form docstrings, but let's see if they have a configuration to avoid this.

@dwreeves
Copy link
Contributor Author

I do believe the legacy version (default handler) is wrong, and that the experimental version (the handler with mkdocstrings-python==0.6.6 in the requirements) is correct. It may be of use for the mkdocstrings team to be aware of this particular situation and give an option for people control what happens, but I think griffe's current default behavior makes sense to me.

The reason why it's wrong to use wtforms.Form's docstrings is because form_base_class is (1) an alias for another class, and (2) it has its docstring being explicitly overridden by the developer, i.e. the docstring below its definition signals intent from the developer to use a different docstring than wtforms.Form.__doc__.


I think the more pressing issue is that the mkdocstrings-python handler does not have a members attribute at the moment. You can see here that the mkdocstrings legacy handler uses the members attribute, but there is no equivalent in the experimental version yet.

@aminalaee
Copy link
Owner

Well we haven't seen any progress with that issue and both approaches have their problems, I'd say let's comment out form_base_class for now. What do you think?

@dwreeves
Copy link
Contributor Author

dwreeves commented Mar 29, 2022

Two options. Both are fine:

  1. Comment out in the docs.
  2. Have form_base_class's default value be None, and add if self.form_base_class is None: self.form_base_class = Form to the __init__.

Copy link
Owner

@aminalaee aminalaee left a comment

Choose a reason for hiding this comment

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

Thank you for the PR 🎉

@aminalaee aminalaee merged commit 6d2b6cc into aminalaee:main Mar 30, 2022
@aminalaee aminalaee mentioned this pull request Apr 18, 2022
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.

[Bug] Docs are rendering docstrings from wtforms.Form and not form_base_class.
2 participants