Skip to content

Conversation

pierreN
Copy link
Contributor

@pierreN pierreN commented Apr 12, 2020

Adds a functional test that parses available RPC commands, generates the associated bitcoin-cli autocomplete file and checks that the current autocomplete file matches.

An outdated autocomplete file can be updated via the --overwrite test parameter.

The first commit syncs the auto-completion file with the current master.
The second commits adds the test. It just changes the header of the autocomplete file and moves it to test/functional/data/completion/bitcoin-cli.bash-completion (plus for ease of use and backward compatibility a symlink from contrib/bitcoin-cli.bash-completion to the moved file is created).

This PR aims to fix: #17289

@pierreN pierreN changed the title test: check that bitcoin-cli autocomplete is in sync test: checks that bitcoin-cli autocomplete is in sync Apr 12, 2020
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.

Concept ACK

@maflcko maflcko removed the Tests label Apr 12, 2020
@pierreN pierreN force-pushed the feature-auto-cli branch 2 times, most recently from a332a11 to 4576b77 Compare April 12, 2020 21:16
@pierreN
Copy link
Contributor Author

pierreN commented Apr 12, 2020

It seems that even when using SRCDIR CI builds fail: https://travis-ci.org/github/pierreN/bitcoin/jobs/674154306#L3417

A decent way to fix this would be to add the bash auto-completion file to Makefile.am's EXTRA_DIST. I updated the PR to reflect that.

If you think this is too intrusive, another more hackish way would be to just look for the file in the parent directories (https://github.com/pierreN/bitcoin/blob/feature-auto-cli-2/test/functional/tool_cli_completion.py#L260)?

""" Complete the arguments of option via the RPC help command. """

# if we can't find an argument list in the help text, then it has no option to add
regexp_res = ARG_REGEXP.match(self.nodes[0].help(option.command))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
regexp_res = ARG_REGEXP.match(self.nodes[0].help(option.command))
res = self.nodes[0].format(command=option.command, output=args_cli)

Instead of using regex to parse the pseudo doc-json, a hidden RPC could be added that spits out the format you want.

I think all you need to do is register a formatter in RPCMan::Check

Copy link
Contributor Author

@pierreN pierreN Apr 14, 2020

Choose a reason for hiding this comment

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

Yes, good idea. I have updated the branch to do that. Please have a look at the new code, thanks.

Note that some non-hidden RPC calls didn't use RPCHelpMan::Check so I added a commit to fix that first (otherwise they wouldn't appear in the bash cli completion file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't notice #18531 when writing 412bb2f - I'll rebase this PR after #18531 is merged.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 14, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

pierrenn added 3 commits April 14, 2020 21:44
- add format command to get infos about commands via a particular format
- add output format args_cli to get arguments type info of shown commands
- refactor RPCArg::ToTypeString to be used accross multiple output formats
- overload CRPCTable::execute to call != methods than in their request
Add a functional test which parses available RPC commands, generates
the associated bitcoin-cli autocomplete file and checks that the current
autocomplete file matches.
An outdated autocomplete file can be updated via the --overwrite test
parameter.
@pierreN
Copy link
Contributor Author

pierreN commented Apr 14, 2020

Updated branch to partially get rid of conflict with #17356 (some conflict might be left if git is not clever enough).

Added a comment for #12674 : https://github.com/bitcoin/bitcoin/pull/12674/files#r408113386

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

@adamjonas
Copy link
Member

@pierreN is this something you plan on continuing to work on?

@adamjonas
Copy link
Member

@pierreN ping for rebase.

@pierreN
Copy link
Contributor Author

pierreN commented May 6, 2021

Sorry for the few months delay.
I have a bit more time now and will try to follow through with this PR.

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

# If you want to modify this file, modify test/functional/tool_cli_completion.py and re-autogenerate
# this file via the --overwrite test flag.

# Copyright (c) 2012-2020 The Bitcoin Core developers

Choose a reason for hiding this comment

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

Happy New Year 🥂

Suggested change
# Copyright (c) 2012-2020 The Bitcoin Core developers
# Copyright (c) 2012-2022 The Bitcoin Core developers

@niVelion
Copy link

niVelion commented Jan 14, 2022

ACK, would be great to see this merged.

I can see in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits that the next task is primarily to amend the commit message after squashing, the message that'll make it into the master branch to describe this improvement.

Here's the starting point:

# This is a combination of 4 commits.
# This is the 1st commit message:

rpc: make all unhidden actors use RPCHelpMan::Check

# This is the commit message #2:

rpc: add format command with support for args_cli

- add format command to get infos about commands via a particular format
- add output format args_cli to get arguments type info of shown commands
- refactor RPCArg::ToTypeString to be used accross multiple output formats
- overload CRPCTable::execute to call != methods than in their request

# This is the commit message #3:

contrib: sync bitcoin-cli bash autocompletion

# This is the commit message #4:

test: keeps bitcoin-cli autocomplete in sync

Add a functional test which parses available RPC commands, generates
the associated bitcoin-cli autocomplete file and checks that the current
autocomplete file matches.
An outdated autocomplete file can be updated via the --overwrite test
parameter.

@fanquake
Copy link
Member

#25243 has picked this up.

@bitcoin bitcoin locked and limited conversation to collaborators May 30, 2023
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.

contrib: Autogenerate bash completion
6 participants