Skip to content

Always transplant a stub for transplanted methods#2427

Merged
lilyminium merged 23 commits intodevelopfrom
feature/more-transplant
Oct 22, 2020
Merged

Always transplant a stub for transplanted methods#2427
lilyminium merged 23 commits intodevelopfrom
feature/more-transplant

Conversation

@jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Dec 25, 2019

Fixes #1845

Methods and properties defined by topology attributes and transplanted in various core classes (groups and Universe) do not appear in the documentation. Several solutions have been proposed to solve this problem:

Here, instead, I systematically transplant a dummy method in the core class for each method that is to be transplanted. The dummy method mimics the name, signature, and documentation of the original method, but always raises a NoDataError. The dummy methods (stubs) are picked up by sphinx and appear in the documentation. Also, the transplant methods from topology attributes that are not present in a given universe do not raise a confusing AttributeError any more, instead, they raise a NoDataError that specify which attribute is missing.

Compared to #2418, this method does not produce the cool looking tables and does not automatically tell that an attribute is required in the doc. It also has the same issue with reference duplicates (see #2176 (comment)). However, it does not require any manual operation.

The problem with the references is about one citation only, though.

Compared to #2424, it keeps all the benefit of the transplant as described in #601.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@jbarnoud
Copy link
Contributor Author

I will move the transplant of the stub in the metaclass to keep all the logic together.

@codecov
Copy link

codecov bot commented Dec 25, 2019

Codecov Report

Merging #2427 into develop will decrease coverage by 0.00%.
The diff coverage is 95.12%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2427      +/-   ##
===========================================
- Coverage    93.05%   93.05%   -0.01%     
===========================================
  Files          186      186              
  Lines        24611    24651      +40     
  Branches      3188     3194       +6     
===========================================
+ Hits         22902    22938      +36     
- Misses        1661     1665       +4     
  Partials        48       48              
Impacted Files Coverage Δ
package/MDAnalysis/core/topologyattrs.py 96.44% <95.00%> (-0.06%) ⬇️
package/MDAnalysis/core/groups.py 98.58% <100.00%> (ø)
package/MDAnalysis/transformations/translate.py 95.23% <0.00%> (-4.77%) ⬇️

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 beb5232...16dcb2d. Read the comment docs.

Topology attributes can transplant methods in other classes (mostly the
groups). This transplant, however only happens when a given attribute is
attached to a topology. This causes issues including missing
documentation for the transplanted methods and inonsitent error
messages.

This commit transplants a dummy method fo all transplants. This happens
independently from the unverse creation, so the stubs can appear in the
documentation. The stubs carry the docstring of their counterpart
methods, and raise a NoDataError when called.
Add funcsigs as a dependancy.
This avoids issues in sphinx about duplicate references in the stubs
docstrings.
@jbarnoud jbarnoud force-pushed the feature/more-transplant branch from 64853db to db52364 Compare October 18, 2020 13:34
@pep8speaks
Copy link

pep8speaks commented Oct 18, 2020

Hello @jbarnoud! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 612:80: E501 line too long (81 > 79 characters)

Comment last updated at 2020-10-20 08:27:05 UTC

@jbarnoud
Copy link
Contributor Author

This is what it looks like:

mda-doc

@jbarnoud jbarnoud changed the title [WIP] Always transplant a stub for transplanted methods Always transplant a stub for transplanted methods Oct 18, 2020
@jbarnoud
Copy link
Contributor Author

I think this is ready for review. Maybe @lilyminium would have opinions about it.

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

This looks great, thank you @jbarnoud! However, if it's not too much work to bring the Python 2 funcsigs back in, it would be great to backport this PR to master so the transplanted methods are documented in the 1.0.x docs too -- the methods do exist in that version as well.

Fix spelling and typos

Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>
@jbarnoud
Copy link
Contributor Author

@lilyminium I added back funcsign and got rid of an f-string to help with backport. The patch should work on python 2.7, but I did not try as the rest of develop doesn't. The uncovered code is the import of funcsign as it would only run on python 2.

@lilyminium lilyminium merged commit 799100b into develop Oct 22, 2020
@lilyminium lilyminium deleted the feature/more-transplant branch October 22, 2020 08:24
@lilyminium
Copy link
Member

Thank you @jbarnoud for this nice solution!

@lilyminium lilyminium mentioned this pull request Oct 23, 2020
@orbeckst
Copy link
Member

Thank you @jbarnoud – makes me happy that this is working.

We can remove the funcsig stuff again – but it makes it easier to backport because we can cherry pick.

orbeckst pushed a commit that referenced this pull request Oct 23, 2020
- Fixes #1845
- Adds dummy method with documentation of original method
- Dummy method gets replaced by actual method on instantiation

(cherry picked from commit 799100b)
orbeckst added a commit that referenced this pull request Oct 23, 2020
backport of stub transplants of AtomGroup (PR #2427)
PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021
- Fixes MDAnalysis#1845 
- Adds dummy method with documentation of original method
- Dummy method gets replaced by actual method on instantiation
@IAlibay IAlibay mentioned this pull request May 13, 2021
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.

docs for many AtomGroup methods missing

5 participants