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

Doctests for Account and other general doc cleanup #94

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

kclowes
Copy link
Collaborator

@kclowes kclowes commented Mar 31, 2020

What was wrong?

There were no doctests in the eth-account docs.

Issue #9

How was it fixed?

Added some!

Cute Animal Picture

image

@kclowes kclowes changed the title [WIP] Doctests for Account and other general doc cleanup Doctests for Account and other general doc cleanup Mar 31, 2020
Copy link
Contributor

@carver carver left a comment

Choose a reason for hiding this comment

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

LGTM. I'm assuming that you ran the tests locally, and that was part of how you found all the little fixups. Were you able to test that CircleCI is running doctest correctly? (ie~ push a doc that should fail in CI to make sure it turns red)

Since this is the first time turning on doctests, AFAIK, would be nice to have a quick gut-check that everything is wired together.

@@ -46,7 +46,7 @@ def address(self):
@property
def privateKey(self):
"""
.. CAUTION:: Deprecated for :var:`~eth_account.signers.local.LocalAccount.key`.
.. CAUTION:: Deprecated for :meth:`~eth_account.signers.local.LocalAccount.key`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah interesting, so the link changes to :meth: when it's a @property. Did this show up during doc tests? You were able to confirm that it links correctly with this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, it was showing up as a warning in the doc build. And it does work correctly!

@kclowes
Copy link
Collaborator Author

kclowes commented Apr 3, 2020

@carver I had run the tests using the circleci CLI locally, but not all together, so I'm glad I did that. Good suggestion! It looks like the integration and core tests were running tests in the eth_account directory as well, so I took those out from the tox.ini (see the last commit), but let me know if that doesn't sound right to you.

@carver
Copy link
Contributor

carver commented Apr 3, 2020

I had a vague memory that they were intentional. Luckily it doesn't change too often, so a git blame quickly showed the commit that added it: 8d99dea

It does look like it's necessary to leave it in so that doctest runs against any docstring examples. I think the inclusion in the integration test is just copy/pasta though:
780a7cf#diff-b91f3d5bd63fcd17221b267e851608e8

Although, one option would be to leave the change you did already and then add an extra tox.ini command under the doctest environment: pytest eth_account.

That way any docstring doctest failures would show up in the doctest CI job instead of the core job. That's probably more intuitive.

@pipermerriam pipermerriam merged commit 91f8c6c into ethereum:master Apr 21, 2020
pacrob added a commit to pacrob/eth-account that referenced this pull request Jan 30, 2024
change references to doc to all be docs
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.

3 participants