Skip to content

[travis] Fail when documentation is outdated #7280

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 19, 2016
Merged

[travis] Fail when documentation is outdated #7280

merged 2 commits into from
Jan 19, 2016

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 4, 2016

This checks if all command line args are documented in the help message.

Example output:

$ qa/pull-tester/check-doc.py 
Args used        : 144
Args documented  : 133
Args undocumented: 15
set(['-uacomment', '-nodebug', '-rpcssl', '-rpccookiefile', '-checkblockindex', '-benchmark', '-help', '-version', '-h', '-checkmempool', '-mocktime', '-maptxfee', '-tor', '-socks', '-debugnet'])
Args unknown     : 4
set(['-zmqpubrawblock', '-zmqpubhashtx', '-zmqpubrawtx', '-zmqpubhashblock'])
$ echo $?
15

@dcousens
Copy link
Contributor

dcousens commented Jan 4, 2016

concept ACK

@jonasschnelli
Copy link
Contributor

Nice!
We just need to be clear: this would mean, we extend the CI/pull-tester from functional code testing to style/documentation quality assurance.

IMO, this is something we should do.

utACK fa0f5100493572ea7abd2b0a9d98c697adc31014.

jonasschnelli
jonasschnelli reviewed Jan 4, 2016
View reviewed changes
qa/pull-tester/check-doc.py
@@ -0,0 +1,39 @@
#!/usr/bin/env python
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add copyright header (https://github.com/MarcoFalke/bitcoin/blob/MarcoFalke-2015-travisDoc/qa/pull-tester/rpc-tests.py#L2)

Copy link
Member

Choose a reason for hiding this comment

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

Concept ACK

On Monday, 4 January 2016, Jonas Schnelli notifications@github.com wrote:

In qa/pull-tester/check-doc.py
#7280 (comment):

@@ -0,0 +1,39 @@
+#!/usr/bin/env python
+'''

nit: add copyright header (
https://github.com/MarcoFalke/bitcoin/blob/MarcoFalke-2015-travisDoc/qa/pull-tester/rpc-tests.py#L2
)

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/7280/files#r48710140.

laanwj
laanwj reviewed Jan 4, 2016
View reviewed changes
qa/pull-tester/check-doc.py
REGEX_DOC = re.compile(r'HelpMessageOpt\(\"(\-[^\"=]+?)(?:=|\")')

def main():
used = check_output(CMD_GREP_ARGS, shell=True)
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to never use any of the subprocess commands with shell=True, using a shell can easily lead to security issues.

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 agree that shell=True should never be used. But I am just too lazy to translate the pipe into python. The CMD_s are hardcoded, so shell injection is rather unlikely to happen.

@laanwj
Copy link
Member

laanwj commented Jan 4, 2016

Concept ACK
It's quite ugly to do this as long as there is no organization to the argument parsing, but as long as the risk of false positives is low it's good to add the check.

@maflcko maflcko force-pushed the MarcoFalke-2015-travisDoc branch from fa0f510 to 7497b86 Compare January 4, 2016 18:20
@maflcko maflcko force-pushed the MarcoFalke-2015-travisDoc branch from 7497b86 to fada0c9 Compare January 4, 2016 18:24
@maflcko
Copy link
Member Author

maflcko commented Jan 4, 2016

Moved check-doc.py to devtools and addressed nit by @jonasschnelli

@laanwj
Copy link
Member

laanwj commented Jan 7, 2016

Travis:

/home/travis/build.sh: line 45: contrib/devtools/check-doc.py: No such file or directory

@maflcko maflcko force-pushed the MarcoFalke-2015-travisDoc branch from 128e7f0 to a1c905c Compare January 7, 2016 15:15
@maflcko maflcko force-pushed the MarcoFalke-2015-travisDoc branch from a1c905c to faeda0e Compare January 18, 2016 13:01
@maflcko
Copy link
Member Author

maflcko commented Jan 18, 2016

Documentation was added in master, so travis should pass now.

@laanwj laanwj merged commit faeda0e into bitcoin:master Jan 19, 2016
laanwj added a commit that referenced this pull request Jan 19, 2016
@laanwj
Merge pull request #7280
668906f 
faeda0e [travis] Run contrib/devtools/check-doc.py early (MarcoFalke)
fada0c9 [travis] Fail when documentation is outdated (MarcoFalke)
@maflcko maflcko deleted the MarcoFalke-2015-travisDoc branch January 19, 2016 15:52
codablock pushed a commit to codablock/dash that referenced this pull request Dec 9, 2017
@laanwj @codablock
Merge pull request bitcoin#7280
42997ee 
faeda0e [travis] Run contrib/devtools/check-doc.py early (MarcoFalke)
fada0c9 [travis] Fail when documentation is outdated (MarcoFalke)
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maflcko @dcousens @jonasschnelli @laanwj @fanquake