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

Add option to document properties using autoproperty instead of autoattribute #197

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

WilliamJamieson
Copy link

Currently attributes and properties are treated the same by automodapi when it comes to generating documentation. This is problematic if one wants to do things like have the return type of a property type hint to be included in the documentation. The autoproperty directive can do this, where as the autoattribute cannot.

This PR adds the config option automodsumm_properties_are_attributes which when True(the default) treats all properties like they are attributes and when False treats them using the autoproperty directive.

@pllim pllim added this to the 0.19.0 milestone Jan 10, 2025
@WilliamJamieson
Copy link
Author

To see the difference, before these changes:
Before
vs after these changes with automodsumm_properties_are_attributes=False:
After

@pllim
Copy link
Member

pllim commented Jan 10, 2025

Might wanna wait for #196 to make sure this actually passes with sphinx-dev

@WilliamJamieson WilliamJamieson marked this pull request as ready for review January 10, 2025 19:53
Comment on lines +629 to +647
public_properties, all_properties = \
get_members_class(obj, 'property',
include_base=include_base)
if properties_are_attributes:
ns['attributes'].extend(public_properties)
ns['all_attributes'].extend(all_properties)
else:
ns['properties'] = public_properties
ns['all_properties'] = all_properties
Copy link
Author

Choose a reason for hiding this comment

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

This is the real change.

@bsipocz
Copy link
Member

bsipocz commented Jan 10, 2025

Want to revisit #134 after this, too and then we can cut a release?

@Cadair
Copy link
Member

Cadair commented Jan 10, 2025

I am pro making this new behavior the default. @WilliamJamieson said to me that automodapi predates autoproperty in sphinx. I see no good reason why we should use this everywhere.

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.05%. Comparing base (594fc12) to head (09aeffb).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...automodapi/tests/example_module/attribute_class.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
- Coverage   90.29%   90.05%   -0.25%     
==========================================
  Files          30       31       +1     
  Lines        1206     1227      +21     
==========================================
+ Hits         1089     1105      +16     
- Misses        117      122       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pllim
Copy link
Member

pllim commented Jan 10, 2025

@WilliamJamieson , please rebase to pick up devdeps fixes. Thanks!

making this new behavior the default

Might be surprising for downstream packages. Are you sure?!

@Cadair
Copy link
Member

Cadair commented Jan 13, 2025

Might be surprising for downstream packages. Are you sure?!

Sure things change when you upgrade packages. I think this is a straight upgrade, with no real downside other than "different". If we want to we could make it throw a warning until someone explicitly sets the config flag?

@pllim
Copy link
Member

pllim commented Jan 13, 2025

Given @eteq originally wrote this and @astrofrog then patched it (9 years ago!), maybe they have opinions?

If not, maybe at least a draft PR to astropy with this branch to compare before merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants