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

Refactor the ASTBuilder to get rid of the currentAttr attribute. #585

Merged
merged 21 commits into from
Sep 22, 2024

Conversation

tristanlatr
Copy link
Contributor

@tristanlatr tristanlatr commented May 20, 2022

Refactor the ASTBuilder to get rid of the currentAttr attribute.

This attribute was used to attach the right docstring node to the right Attribute object. Now it uses AST node navigation (with the .parent attribute) instead for fetching the docstring node for an ast.Assign.

This change removes a attribute being mutated at different places in the code, replaces this kind of "unsafe" state tracking (meaning not with pop() and push()) by some more explicit solution.

The zopeinferface extension needed to be adjusted as well because it relied on the docstring assignment feature in an implicit way, now it's explicit what we're doing.

This attribute was used to attach the right docstring node to the right Attribute object. Now it uses AST node navigation (with the .parent attribute) instead for fetching the docstring node for an ast.Assign.

This change might not be worth it, on the one hand it removes a attribute beeing mutated at different palces in the code, but replaces this kind of "unsafe" state tracking (meaning not with pop() and push()) by some more verbose solution that involves adding the .parent attribute on all nodes.

The zopeinferface extension needed to be adjusted as well because it relied on the docstring assigment feature in an implicit way, now it's explicit what we're doing.
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Attention: Patch coverage is 87.87879% with 12 lines in your changes missing coverage. Please review.

Project coverage is 92.03%. Comparing base (0d07110) to head (52f6a15).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pydoctor/astbuilder.py 92.95% 4 Missing and 1 partial ⚠️
pydoctor/astutils.py 77.27% 3 Missing and 2 partials ⚠️
pydoctor/extensions/zopeinterface.py 66.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
- Coverage   92.07%   92.03%   -0.04%     
==========================================
  Files          47       47              
  Lines        8412     8465      +53     
  Branches     1856     1868      +12     
==========================================
+ Hits         7745     7791      +46     
- Misses        391      395       +4     
- Partials      276      279       +3     

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

@tristanlatr
Copy link
Contributor Author

tristanlatr commented May 22, 2022

After a second though, I believe it’s worth it.

Mainly because:

  • it’s now more explicit what happens in the zope interface extension.

  • Narrowing down the ASTBuilder and ModuleVistor interface is a good thing since it’s used in customization mechanism.

  • Also the parentage feature of the ast tree makes our builder more powerful.

pydoctor/astbuilder.py Outdated Show resolved Hide resolved
pydoctor/astbuilder.py Outdated Show resolved Hide resolved
@tristanlatr tristanlatr requested a review from glyph June 11, 2022 17:37
pydoctor/astbuilder.py Outdated Show resolved Hide resolved
pydoctor/astbuilder.py Outdated Show resolved Hide resolved
pydoctor/astbuilder.py Outdated Show resolved Hide resolved
pydoctor/astbuilder.py Outdated Show resolved Hide resolved
pydoctor/astbuilder.py Show resolved Hide resolved
@glyph glyph removed their request for review February 6, 2023 23:02
@glyph
Copy link
Member

glyph commented Feb 6, 2023

@tristanlatr sorry to punt on this, but can you address the conflicts and then re-request from @twisted/twisted-contributors rather than me personally?

This comment has been minimized.

1 similar comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

1 similar comment

This comment has been minimized.

pydoctor/astbuilder.py Outdated Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

@tristanlatr
Copy link
Contributor Author

This PR is ready to be merged. It does not worth a changelog entry since it's only refactoring. It also fixes a edge case bug that probably got unnoticed.

@tristanlatr tristanlatr requested a review from a team September 15, 2024 13:03

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pydoctor/astutils.py Outdated Show resolved Hide resolved
pydoctor/astutils.py Outdated Show resolved Hide resolved
Copy link

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

numpy (https://github.com/numpy/numpy)
- /projects/numpy/numpy/distutils/ccompiler_opt.py:1038: Existing docstring at line 949 is overriden
- /projects/numpy/numpy/distutils/ccompiler_opt.py:1075: Cannot find link target for "bool"
+ /projects/numpy/numpy/distutils/ccompiler_opt.py:986: Cannot find link target for "bool"

@tristanlatr tristanlatr merged commit 5be6898 into master Sep 22, 2024
41 checks passed
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