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

Better property support #640

Open
wants to merge 61 commits into
base: master
Choose a base branch
from
Open

Conversation

tristanlatr
Copy link
Contributor

The property setter and deleter informations now shows at the same place as the property getter. Meaning inside the documentation of an Attribute object.

Fixes #587. Together with #595 it will look better (without the Undocumented param tables).

As a side effect of the change, the setters and deleters Function are removed from the documentable tree, we can't look for the property setters in the search bar with *.setter, we can't link to them from docstrings (but still we can link to them with their anchor) and they won't display in intersphinx mapping.

This is a breaking change, but that might not be very important. These objects do not actually exists at runtime, so I think it's OK to remove them from the tree.

Before:

by default 2022-08-05 at 12 38 18 AM

After:

by default 2022-08-05 at 12 23 58 AM

@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (9becf85) 92.69% compared to head (1bb7bed) 92.70%.

Files Patch % Lines
pydoctor/astbuilder.py 93.75% 4 Missing and 3 partials ⚠️
pydoctor/model.py 91.89% 1 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #640      +/-   ##
==========================================
+ Coverage   92.69%   92.70%   +0.01%     
==========================================
  Files          47       47              
  Lines        8285     8461     +176     
  Branches     1826     1872      +46     
==========================================
+ Hits         7680     7844     +164     
- Misses        347      352       +5     
- Partials      258      265       +7     

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

Copy link
Contributor Author

@tristanlatr tristanlatr left a comment

Choose a reason for hiding this comment

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

More tests should be added to ensure we’re a not crashing with KeyError when objects are getting renamed because there are duplicates or moved around because they’re reparented.


@property
def undoc_prop(self) -> bytes:
"""This property has a docstring only on the setter."""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"""This property has a docstring only on the setter."""
"""This property has a docstring only on the getter."""

docs/epytext_demo/demo_epytext_module.py Outdated Show resolved Hide resolved
pydoctor/model.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@tristanlatr tristanlatr left a comment

Choose a reason for hiding this comment

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

Few things needs to be improved here:

  • Trigger warnings when pydoctor doesn't understand a property def
  • Probably the code can be factored into less lines

pydoctor/astbuilder.py Outdated Show resolved Hide resolved
pydoctor/model.py Outdated Show resolved Hide resolved
builder.addModuleString(base_mod, modname='mod')
builder.buildModules()

assert not capsys.readouterr().out
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should trigger a few warnings.

@tristanlatr
Copy link
Contributor Author

There are several refactors to do for the changes to be clean, it should take less lines and function should created to wrap the properly handing logic while visiting the function defs.

And more importantly, I’m unsure if it worth it to support inherited property overrides, like the proposed change currently does. On the one hand, it’s properly covered by tests cases and it’s used in cpython tests, but on the other hand, it’s not very common usage of te property decorators and it adds about 80 lines of code to pydoctor. But I think it’s can’t be bad to support it at the end of the day it’s safe and it doesn’t make pydoctor crash.

@tristanlatr tristanlatr marked this pull request as draft January 18, 2023 18:05
@tristanlatr
Copy link
Contributor Author

I think I'll remove the support for inherited property overrides. It's jut (almost) never used in real life code. I only ever saw that in cpython test cases.

This have been done. At the same time, I've added support for property(fset, fget, fdel, doc) old school decoration.

This comment has been minimized.

This comment has been minimized.

@tristanlatr
Copy link
Contributor Author

The numpy diff bugs me... I'm not currently sure the cause of such a diff.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

…ated with the property() call.

Simpler message when a docstring is getting overriden.
Copy link

Diff from pydoctor_primer, showing the effect of this PR on open source code:

astroid (https://github.com/pylint-dev/astroid)
+ /projects/astroid/astroid/nodes/scoped_nodes/scoped_nodes.py:263: Existing docstring at line 260 is overriden
+ /projects/astroid/astroid/nodes/scoped_nodes/scoped_nodes.py:1871: Existing docstring at line 1773 is overriden
+ /projects/astroid/astroid/nodes/scoped_nodes/scoped_nodes.py:1910: Existing docstring at line 1854 is overriden
+ /projects/astroid/astroid/nodes/scoped_nodes/scoped_nodes.py:1875: Parameter name missing
+ /projects/astroid/astroid/nodes/scoped_nodes/scoped_nodes.py:2003: Parameter name missing
+     astroid.nodes.Arguments.arguments.getter
-     astroid.nodes.node_classes.Arguments.arguments

pycma (https://github.com/CMA-ES/pycma)
+ /projects/pycma/cma/sampler.py:249: Existing docstring at line 244 is overriden

twisted (https://github.com/twisted/twisted): typechecking got 1.08x slower (235.2s -> 252.9s)
(Performance measurements are based on a single noisy sample)

numpy (https://github.com/numpy/numpy)
- /projects/numpy/numpy/ma/core.py:2745: Cannot find link target for "array_like"
+ /projects/numpy/numpy/ma/core.py:3718: Cannot find link target for "numpy.ndarray"

@tristanlatr
Copy link
Contributor Author

Blockers:

  • The warning /projects/astroid/astroid/nodes/scoped_nodes/scoped_nodes.py:1875: Parameter name missing is a bug and should be fixed.
  • model.Property should be removed, we should not have to handle yet another documentable type in the customizations nor in our code. What I think would be smart to do is to derive from System.Attribute at runtime in order to dynamically create the type System.Property. We could do about the same for Alias handling.

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.

Unify properties getter, setters and deleters under a single documentation entry
1 participant