-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Fix docstrings in qa tests #9577
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
Conversation
|
Thanks for adding documentation. |
|
utACK 077b626 |
|
Fast review ACK 077b626 |
|
Needs rebase |
|
rebased |
|
@MarcoFalke you have tagged yourself as reviewer, this can be merged as soon as you are ok with it |
|
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. |
|
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. |
qa/rpc-tests/create_cache.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing a trailing e?
qa/rpc-tests/disablewallet.py
Outdated
There was a problem hiding this comment.
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.
maflcko
left a comment
There was a problem hiding this 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.
|
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. |
|
@jnewbery Thanks. |
ryanofsky
left a comment
There was a problem hiding this 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.
qa/rpc-tests/smartfees.py
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace
There was a problem hiding this comment.
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.
|
Thanks @ryanofsky . Nits addressed and commits squashed. I think this is now ready to merge. |
3f95a80 Fix docstrings in qa tests (John Newbery)
3f95a80 Fix docstrings in qa tests (John Newbery)
* 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
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.