Skip to content

Improve docstrings in offsetbox.py #16085

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

Merged
merged 2 commits into from
Jan 6, 2020
Merged

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Jan 3, 2020

PR Summary

  • numpydoc conforming
  • extended various descriptions
  • Fixed some doc errors in other modules, which I found along the way.

@timhoffm timhoffm added this to the v3.3.0 milestone Jan 3, 2020
@@ -1245,6 +1279,9 @@ def _get_anchored_bbox(self, loc, bbox, parentbbox, borderpad):
class AnchoredText(AnchoredOffsetbox):
"""
AnchoredOffsetbox with Text.

This is a convenience class. Technically, it is just an `AnchoredOffsetbox`
containing a `TextArea`.
Copy link
Contributor

Choose a reason for hiding this comment

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

last sentence basically duplicates the first line...

Copy link
Member Author

Choose a reason for hiding this comment

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

The first line gives the intent of the class. The last sentence describes how it it realized. I think both are helpful.

Copy link
Contributor

@anntzer anntzer Jan 4, 2020

Choose a reason for hiding this comment

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

Something like
Convenience subclass of `AnchoredOffsetbox` for holding a `TextArea`.
would be enough, and fit in one line?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that catches the essence. From the outside it's really created like a text object: the text s is the first parameter. Only if I'm to operate on it further, it helps to know that it's ans AnchoredOffsetbox containing a TextArea.

If you strictly want a single line, I'd rather go back to the original docstring.

TBH, I don't really understand all this offsetbox business. Maybe it's just a lack of documentation, but it feels overly complicated for what it's trying to achieve.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand it completely either, but I think I would rather leave things as is and first clarify the actual intent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted for now. Thanks for the discussion.

@anntzer
Copy link
Contributor

anntzer commented Jan 4, 2020

Looks like you messed up your rebase.

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

postci

@anntzer
Copy link
Contributor

anntzer commented Jan 6, 2020

@timhoffm I'll let you squash-merge? (or just merge if you really want both commits)

@timhoffm timhoffm merged commit 7c119b6 into matplotlib:master Jan 6, 2020
@timhoffm timhoffm deleted the doc-offsetbox branch January 6, 2020 10:35
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.

2 participants