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

GenericMap now passes docstrings to its subclasses #5122

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

asinghgaba
Copy link
Contributor

Description

Helps GenericMap to pass its docstring to its subclasses.
Fixes #3716

@asinghgaba asinghgaba requested a review from a team as a code owner March 20, 2021 22:07
@pep8speaks
Copy link

pep8speaks commented Mar 20, 2021

Hello @asinghgaba! Thanks for updating this PR.

Line 2510:101: E501 line too long (223 > 100 characters)
Line 2257:101: E501 line too long (103 > 100 characters)
Line 2175:101: E501 line too long (103 > 100 characters)
Line 1975:101: E501 line too long (121 > 100 characters)
Line 1872:101: E501 line too long (139 > 100 characters)
Line 1717:101: E501 line too long (109 > 100 characters)
Line 1660:101: E501 line too long (106 > 100 characters)
Line 1659:101: E501 line too long (108 > 100 characters)
Line 1616:101: E501 line too long (114 > 100 characters)
Line 1247:101: E501 line too long (111 > 100 characters)
Line 1032:101: E501 line too long (109 > 100 characters)
Line 1031:101: E501 line too long (131 > 100 characters)
Line 34:101: E501 line too long (109 > 100 characters)

Comment last updated at 2021-04-28 19:33:35 UTC

sunpy/map/mapbase.py Outdated Show resolved Hide resolved
sunpy/map/mapbase.py Outdated Show resolved Hide resolved
@dstansby dstansby added No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) and removed [DocFix] labels Mar 21, 2021
@dstansby
Copy link
Member

I think this is a change in behaviour, not a trivial fix, so I've milestoned for 3.0 instead of backporting.

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Looks good to me - I'm happy to approve when @ayshih suggested change to the order has been made.

ayshih
ayshih previously requested changes Mar 23, 2021
Copy link
Member

@ayshih ayshih left a comment

Choose a reason for hiding this comment

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

I think the output from the PR is confusing (example for CORMap):

  • The initial docstring of GenericMap ("A Generic spatially-aware 2D data array") is not stripped out or otherwise prefaced.
  • The example in the docstring GenericMap is not stripped out, which means that every non-AIAMap class shows an example of using AIAMap. (As an aside, why does the docstring for GenericMap not show GenericMap?)

This demonstrates that we can't do a simple string addition. There needs to be a deeper level of refactoring so that docstring elements can be re-used in the docstrings of both GenericMap and the subclasses.

@asinghgaba
Copy link
Contributor Author

@ayshih I think this should be done by adding docstrings individually to each subclass. We could then strip as much we want according to the subclass. What do you think about doing it this way?

@ayshih
Copy link
Member

ayshih commented Mar 24, 2021

I (mostly) disagree. We want the creation of the merged docstring to be under GenericMap to facilitate maintenance and to ensure a standard presentation. A person creating a new GenericMap subclass should be able to write a simple docstring, and have the merged docstring be automatically created. If we want to support customization for unusual situations, GenericMap.__init_subclass__ can call an overridable class method (e.g., _make_docstring().

@ayshih ayshih marked this pull request as draft March 24, 2021 14:09
@dstansby
Copy link
Member

What do people think of adding some text along the lines of

For more information on map attributes and methods, see documentation for sunpy.map.GenericMap

instead? That way there's still a clear distinction between the docstrings of the two classes, but anyone using help(MyMapSource) has a pointer to GenericMap.

@nabobalis
Copy link
Contributor

I wonder if we want to either do that or break up the important part of the Generic Map docstring and append that. Similar to how we do that with Fido Clients.

@nabobalis nabobalis added this to the 3.0 milestone Apr 7, 2021
@Cadair Cadair added this to Nice to Haves in SunPy 3.0 Apr 22, 2021
@nabobalis nabobalis marked this pull request as ready for review April 23, 2021 14:02
sunpy/map/mapbase.py Outdated Show resolved Hide resolved
@nabobalis nabobalis force-pushed the inherit_doc branch 2 times, most recently from e650155 to 2095e65 Compare April 25, 2021 14:44
sunpy/map/mapbase.py Outdated Show resolved Hide resolved
@Cadair
Copy link
Member

Cadair commented Apr 28, 2021

Rather than taking this approach why not do something similar to what we do in coordinates, where the parameters are templated into the docstrings? Splitting up docstrings on magic sequences feels prone to hard to detect issues to me.

@Cadair Cadair removed this from the 3.0 milestone Apr 28, 2021
@Cadair Cadair removed this from Nice to Haves in SunPy 3.0 Apr 28, 2021
@nabobalis nabobalis removed the request for review from a team April 28, 2021 19:33
@nabobalis nabobalis requested a review from ayshih April 28, 2021 21:09
@nabobalis nabobalis added this to the 3.0 milestone Apr 28, 2021
@nabobalis nabobalis added this to In progress (Blockers) in SunPy 3.0 via automation Apr 28, 2021
@nabobalis nabobalis moved this from In progress (Blockers) to Needs Review in SunPy 3.0 Apr 28, 2021
@nabobalis
Copy link
Contributor

nabobalis commented Apr 29, 2021

Final result: https://sunpy--5122.org.readthedocs.build/en/5122/api/sunpy.map.sources.AIAMap.html#aiamap

There is a double notes section but I think the separation is important?

@Cadair
Copy link
Member

Cadair commented Apr 29, 2021

I don't think the double notes section is an issue, but I am kinda surprised that sphinx doesn't get angry about that, I feel like old numpydoc would have done lol.

@Cadair Cadair merged commit aaa6e2c into sunpy:master Apr 29, 2021
SunPy 3.0 automation moved this from Needs Review to Done Apr 29, 2021
@nabobalis
Copy link
Contributor

Thanks for getting us started on this @asinghgaba!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) No Changelog Entry Needed
Projects
No open projects
SunPy 3.0
  
Done
Development

Successfully merging this pull request may close these issues.

Automatically include Map docstring when displaying help for subclasses?
6 participants