Skip to content

Conversation

@jnewbery
Copy link
Contributor

This commit adds module-level docstrings for the tests and helper
modules in qa. Many of these tests were uncommented previously - this
commit ensures that every test case has at least a minimum level of
commenting.

It also puts the module-level docstring in front of the imports.

@fanquake fanquake added the Docs label Jan 19, 2017
@laanwj
Copy link
Member

laanwj commented Jan 19, 2017

Thanks for adding documentation.
utACK

@maflcko maflcko added this to the 0.15.0 milestone Jan 20, 2017
@maflcko maflcko added the Tests label Jan 20, 2017
@maflcko maflcko self-requested a review January 20, 2017 00:31
@fanquake
Copy link
Member

utACK 077b626

@jtimon
Copy link
Contributor

jtimon commented Feb 1, 2017

Fast review ACK 077b626

@laanwj
Copy link
Member

laanwj commented Feb 20, 2017

Needs rebase

@jnewbery
Copy link
Contributor Author

rebased

@laanwj
Copy link
Member

laanwj commented Feb 21, 2017

@MarcoFalke you have tagged yourself as reviewer, this can be merged as soon as you are ok with it

@ryanofsky
Copy link
Contributor

This does seem like an improvement that should be merged, though it would be nicer if it followed PEP257 to work better with pydoc.

Main reason these strings don't look good in pydoc is they don't follow the guideline to begin with summary lines. A summary line is "a phrase ending in a period. It prescribes the function or method's effect as a command, not as a description." Instead they start with filenames. I think the filenames should be removed even if we don't want to take the time to write summary lines.

@jnewbery
Copy link
Contributor Author

Thanks @ryanofsky . I've made the docstrings conform with PEP257. Can you check the pydocs again?

I've also added docstrings for the modules that have been added since opening this PR.

Copy link
Member

Choose a reason for hiding this comment

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

missing a trailing e?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is not required to be so specific in the doc string. You can leave this link where it was or remove it alltogether.

The test just checks if validateaddress works (even) with the wallet disabled. No need to mention "crash", imo.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

utACK mod 2 nits.

@jnewbery
Copy link
Contributor Author

Thanks for the comments @MarcoFalke - I appreciate this isn't the most thrilling PR to review!

Nits addressed. I will squash when ready to merge.

@maflcko
Copy link
Member

maflcko commented Feb 22, 2017

@jnewbery Thanks.
ACK 82bc966
This is ready for squash.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Again this is already a big improvement as is, so utACK 82bc966aaa9c29555db90f638b3ae65a63d1941d.

But consider running:

sed -i 's/""" \+/"""/' *.py
sed -i 's/\(Test\|Create\)s/\1/g' *.py

to get rid of asymmetric spaces after opening quotes, and to fix verb tenses (commands, not descriptions) for pep257.

Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing whitespace on this line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing whitespace

Copy link
Contributor

Choose a reason for hiding this comment

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

Unintentended space before close quote?

This commit fixes the module-level docstrings for the tests and helper
modules in qa. Many of these tests were uncommented previously - this
commit ensures that every test case has at least a minimum level of
commenting.
@jnewbery
Copy link
Contributor Author

Thanks @ryanofsky . Nits addressed and commits squashed. I think this is now ready to merge.

@maflcko maflcko merged commit 3f95a80 into bitcoin:master Feb 23, 2017
maflcko pushed a commit that referenced this pull request Feb 23, 2017
3f95a80 Fix docstrings in qa tests (John Newbery)
@jnewbery jnewbery deleted the docstrings branch February 23, 2017 18:44
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 3, 2019
3f95a80 Fix docstrings in qa tests (John Newbery)
UdjinM6 pushed a commit to dashpay/dash that referenced this pull request Jan 7, 2019
* Merge bitcoin#9815: Trivial: use EXIT_ codes instead of magic numbers

a87d02a use EXIT_ codes instead of magic numbers (Marko Bencun)

* Merge bitcoin#9801: Removed redundant parameter from mempool.PrioritiseTransaction

eaea2bb Removed redundant parameter from mempool.PrioritiseTransaction (gubatron)

* remove extra parameter (see 3a3745bb) in dash specific code

* Merge bitcoin#9819: Remove harmless read of unusued priority estimates

bc8fd12 Remove harmless read of unusued priority estimates (Alex Morcos)

* Merge bitcoin#9766: Add --exclude option to rpc-tests.py

c578408 Add exclude option to rpc-tests.py (John Newbery)

* Merge bitcoin#9577: Fix docstrings in qa tests

3f95a80 Fix docstrings in qa tests (John Newbery)

* Merge bitcoin#9823: qa: Set correct path for binaries in rpc tests

3333ad0 qa: Set correct path for binaries in rpc tests (MarcoFalke)

* Merge bitcoin#9833: Trivial: fix comments referencing AppInit2

ef9f495 Trivial: fix comments referencing AppInit2 (Marko Bencun)

* Merge bitcoin#9612: [trivial] Rephrase the definition of difficulty.

dc222f8 Trivial: Rephrase the definition of difficulty in the code. (Karl-Johan Alm)

* Merge bitcoin#9847: Extra test vector for BIP32

30aedcb BIP32 extra test vector (Pieter Wuille)

* Merge bitcoin#9839: [qa] Make import-rescan.py watchonly check reliable

864890a [qa] Make import-rescan.py watchonly check reliable (Russell Yanofsky)

Tree-SHA512: ea0e2b1d4fc8f35174c3d575fb751b428daf6ad3aa944fad4e3ddcc9195e4f17051473acabc54203b1d27cca64cf911b737ab92e986c40ef384410652e2dbea1

* Change back file params
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants