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

Extension system #576

Merged
merged 46 commits into from
May 19, 2022
Merged

Extension system #576

merged 46 commits into from
May 19, 2022

Conversation

tristanlatr
Copy link
Contributor

@tristanlatr tristanlatr commented May 12, 2022

Provide a better extension system for pydoctor.

This new extension system breaks some compatibility with the old way of customizing pydoctor with option --system-class because we don't need to subclass the ASTBuilder to include custom AST visitor code. This is why the Twisted build is crashing with this new version. If everything works correctly, the Twisted customizations can be completely removed except the part that adds a link to the latest version of the document we're viewing if it's not the latest version.

The extension system currently support 3 types of customizations:

  • AST builder visitor extensions
  • Mixin classes to be applied to Documentable subclasses
  • Post processors to be run after all modules are built

It does not support support customizing renderers, so the code in ZopeInterfaceClassPage is not part of the extension system, but still built in with pydoctor. This point can probably be taken care of in a later PR.

The extension system rely on composition of multiple visitors into one.
We do not not need to subclass the System class to define an extension, we just need to add a new module under pydoctor.extensions (to be packaged to pydoctor). Though, we currently need to define a system class if we want to add a custom extension.

It now much easier to add more extensions and properly isolate each feature in a different module.

Add a new CLI argument: --extension to load a custom extension. Since it's still possible to add extensions with a custom system like the following:

from pydoctor import model
class MySystem(model.System):
    custom_extensions = ['mylib._pydoctor_extension'] # Load addional extensions

It think I wont add this new argument right now. Also because this change does not mean pydoctor have now a stable API, more changes will come down the road, so I'de suggest we keep the customization system rather "private".

Get rid of attributes currAttr and newAttr and use builder.currentAttr intsead.
This means the extension system is now based on composition of several visitor intead of rely on subclassing.

We cannot test the TwistedSystem anymore because it's not compatible with the new system. It should not be an issue because we now support all the Twisted customizations.
The new extension system is now 100% installed.
@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #576 (79e6404) into master (814db7a) will decrease coverage by 0.04%.
The diff coverage is 93.11%.

@@            Coverage Diff             @@
##           master     #576      +/-   ##
==========================================
- Coverage   90.96%   90.92%   -0.05%     
==========================================
  Files          42       45       +3     
  Lines        7383     7699     +316     
  Branches     1606     1668      +62     
==========================================
+ Hits         6716     7000     +284     
- Misses        403      435      +32     
  Partials      264      264              
Impacted Files Coverage Δ
pydoctor/astutils.py 80.24% <71.42%> (-10.00%) ⬇️
pydoctor/visitor.py 90.83% <90.83%> (ø)
pydoctor/astbuilder.py 94.93% <94.73%> (-0.04%) ⬇️
pydoctor/factory.py 97.01% <97.01%> (ø)
pydoctor/extensions/zopeinterface.py 92.66% <98.03%> (ø)
pydoctor/extensions/__init__.py 98.36% <98.36%> (ø)
pydoctor/extensions/deprecate.py 100.00% <100.00%> (ø)
pydoctor/model.py 93.21% <100.00%> (+0.26%) ⬆️
pydoctor/options.py 94.01% <100.00%> (ø)
pydoctor/templatewriter/pages/__init__.py 82.28% <100.00%> (-2.66%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 814db7a...79e6404. Read the comment docs.

Comment on lines +246 to +259
def _handleZopeInterfaceAssignment(self, node: Union[ast.Assign, ast.AnnAssign]) -> None:
for target in node.targets if isinstance(node, ast.Assign) else [node.target]:
dottedname = astutils.node2dottedname(target)
if dottedname and len(dottedname)==1:
# Here, we consider single name assignment only
current = self.visitor.builder.current
if isinstance(current, model.Class):
self._handleZopeInterfaceAssignmentInClass(
dottedname[0], node.value, node.lineno
)
elif isinstance(current, model.Module):
self._handleZopeInterfaceAssignmentInModule(
dottedname[0], node.value, node.lineno
)
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 new code is needed because we don't subclass the ASTBuilder class anymore, the only entry point we have now is visit_* and depart_* methods. So we need to re-do some semantical analysis job in order to redirect to the correct _handle* method. But it's OK, it's always better than subclassing.

tristanlatr and others added 7 commits May 12, 2022 12:00
… a single object inside one extension.

This also improves coverage.
Fix an issue in the visitor extension logic that would prevent to the depart() method from beeing called if an SkipNode() exception was raised inside the main visitor.
pydoctor/model.py Outdated Show resolved Hide resolved
@tristanlatr
Copy link
Contributor Author

This PR is ready for a final review @adiroiban :)

return None
assert isinstance(cls, ZopeInterfaceClass)
def depart_ClassDef(self, node: ast.ClassDef) -> None:
cls = self.visitor.builder.current.contents.get(node.name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be good to edit the builder to have something like this instead:

Suggested change
cls = self.visitor.builder.current.contents.get(node.name)
cls = self.visitor.builder.last

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll merge this PR as is create a follow-up PR to refactor this, it might more complicated than expected.

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Hi, I am busy these days and will not have time to do a proper review for this one.

I think that we can merge.
if there are any issues, we can fix them later.

overall this looks a big step in the right direction.

Many thanks!

@tristanlatr tristanlatr merged commit 02984d9 into master May 19, 2022
@tristanlatr tristanlatr deleted the extension-system branch May 19, 2022 22:48
@tristanlatr tristanlatr restored the extension-system branch May 20, 2022 00:25
tristanlatr added a commit that referenced this pull request May 24, 2022
…re. The older visitor relied on generic_visit() to be recursive. Where the provided generic_visit() is not recursive anymore, and moreover not called automatically when visiting unknow nodes! So what I'm saying here is that since #576 have been merged, we're not visiting the statements inside the 'orelse' field of Try and If nodes, same goes for 'finalbody' and 'handlers'.

This commit fixes that issue. The rationale is now the following: All statements in the 'orelse' block of IF nodes and statements in the except handlers of TRY nodes that would override a name already defined in the main 'body' (or TRY 'orelse' or 'finalbody') are ignored.

Meaning that in the context of the code below, 'ssl' would resolve to 'twisted.internet.ssl':

try:
    from twisted.internet import ssl as _ssl
except ImportError:
    ssl = None
else:
    ssl = _ssl
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.

2 participants