pydoctor icon indicating copy to clipboard operation
pydoctor copied to clipboard

Refactor the ASTBuilder to get rid of the currentAttr attribute.

Open tristanlatr opened this issue 2 years ago • 3 comments

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 might not be worth it, on the one hand it removes a attribute being mutated at different places 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. (EDIT: we already compute the parent of all nodes)

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.

tristanlatr avatar May 20 '22 06:05 tristanlatr

Codecov Report

Merging #585 (3b00c3e) into master (cd45e05) will decrease coverage by 0.06%. The diff coverage is 86.91%.

@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
- Coverage   90.88%   90.82%   -0.07%     
==========================================
  Files          45       45              
  Lines        7711     7756      +45     
  Branches     1670     1679       +9     
==========================================
+ Hits         7008     7044      +36     
- Misses        438      442       +4     
- Partials      265      270       +5     
Impacted Files Coverage Δ
pydoctor/extensions/zopeinterface.py 91.92% <66.66%> (-0.74%) :arrow_down:
pydoctor/astbuilder.py 94.32% <85.71%> (-0.61%) :arrow_down:
pydoctor/astutils.py 83.17% <92.59%> (+2.93%) :arrow_up:
pydoctor/epydoc/markup/_pyval_repr.py 92.61% <100.00%> (+0.05%) :arrow_up:
pydoctor/model.py 93.21% <100.00%> (ø)

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 cd45e05...3b00c3e. Read the comment docs.

codecov[bot] avatar May 20 '22 06:05 codecov[bot]

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.

tristanlatr avatar May 22 '22 02:05 tristanlatr

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

glyph avatar Feb 06 '23 23:02 glyph