test: autogenerate bash completion #25243

pull suhailsaqan wants to merge 4 commits into bitcoin:master from suhailsaqan:bash-completion changing 12 files +588 −63
  1. suhailsaqan commented at 9:33 AM on May 30, 2022: contributor

    Added a functional test which parses all the RPC help commands then automatically generates the bitcoin-cli bash-completion file and makes sure that the original file matches the newly generated one.

    In order to get the RPC help commands in the correct format needed for the test, I added the "format" RPC command.

    Test using python3 test/functional/tool_cli_completion.py --overwrite

    <!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

  2. fanquake commented at 9:47 AM on May 30, 2022: member

    Thanks for picking this up. Did you re-use any code from #18606? A lot of the changes are very similar to the changes there, except for different whitespace, class names, copyright dates etc. If so, cherry-picking so that the original author is preserved is appropriate.

  3. suhailsaqan force-pushed on May 30, 2022
  4. DrahtBot added the label RPC/REST/ZMQ on May 30, 2022
  5. DrahtBot added the label Scripts and tools on May 30, 2022
  6. suhailsaqan force-pushed on May 30, 2022
  7. laanwj commented at 12:59 PM on May 30, 2022: member

    Concept ACK on auto-generating what can be auto-generated.

    I'm not entirely sure about the approach of making this a functional test. It's clever, but also different from how other auto-generated files (the manual pages, the example bitcoin.conf introduced in #22235) are handled in the release process.

    On the other hand by adding the check this forces the file on the branch to always be up-to-date so it doesn't need to be in the release process. That's also something.

  8. suhailsaqan force-pushed on May 30, 2022
  9. suhailsaqan force-pushed on May 30, 2022
  10. suhailsaqan commented at 1:48 PM on May 30, 2022: contributor

    Concept ACK on auto-generating what can be auto-generated.

    I'm not entirely sure about the approach of making this a functional test. It's clever, but also different from how other auto-generated files (the manual pages, the example bitcoin.conf introduced in #22235) are handled in the release process.

    On the other hand by adding the check this forces the file on the branch to always be up-to-date so it doesn't need to be in the release process. That's also something.

    Look at #17289#issue-513520983

  11. maflcko commented at 2:01 PM on May 30, 2022: member

    Yeah, I wrote that comment before bitcoin.conf moved over to auto-generation. A functional test is one way to achieve this, but this can also be achieved by an auto-generation script at release time.

  12. laanwj commented at 3:59 PM on May 30, 2022: member

    I mean you can argue this is a better solution, and that we should move to this for all three. (but for the manual pages it's difficult as they contain a version number as well as the current month)

  13. brunoerg commented at 5:40 PM on May 30, 2022: contributor

    From linter:

    File "test/functional/tool_cli_completion.py" contains a shebang line, but has the file permission 644 instead of the expected executable permission 755. Do "chmod 755 test/functional/tool_cli_completion.py"
  14. suhailsaqan commented at 12:25 AM on May 31, 2022: contributor

    I mean you can argue this is a better solution, and that we should move to this for all three. (but for the manual pages it's difficult as they contain a version number as well as the current month)

    Thanks for the review @laanwj! Just wondering, do the three of them have to be done the same way? I don't know how I would do it the same way it was done in #22235.

  15. suhailsaqan force-pushed on May 31, 2022
  16. suhailsaqan force-pushed on Jun 3, 2022
  17. suhailsaqan force-pushed on Jun 3, 2022
  18. suhailsaqan commented at 5:03 AM on June 3, 2022: contributor

    Squashed commits.

  19. suhailsaqan force-pushed on Jun 3, 2022
  20. suhailsaqan force-pushed on Jun 3, 2022
  21. laanwj commented at 1:06 PM on June 7, 2022: member

    do the three of them have to be done the same way?

    Not necessarily, but some structure/standardization around release processes makes it harder to forget any steps, than if everything needs to be done in a different way.

  22. luke-jr commented at 12:56 AM on June 18, 2022: member

    On the other hand by adding the check this forces the file on the branch to always be up-to-date so it doesn't need to be in the release process. That's also something.

    If we're not careful, it could also increase the rate of diff conflicts (and/or silent merge bugs?).

  23. DrahtBot commented at 1:39 AM on June 21, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK laanwj

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27788 (rpc: Be able to access RPC parameters by name by achow101)
    • #27432 (contrib: add tool to convert compact-serialized UTXO set to SQLite database by theStack)
    • #26485 (RPC: Accept options as named-only parameters by ryanofsky)

    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.

  24. hernanmarino commented at 4:24 PM on August 12, 2022: contributor

    Hi everyone, I have a questions / suggestion: I believe the object of this issue was to keep the bash files up to date, but this PR only adds a test to check for the file, or optionally update it. Is there something that can be done to make sure that the file is updated or generated on every compile, perhaps by modifying the Makefile to run this test everytime? In that case, I believe that the "--overwrite" option should be the default.

  25. in src/rpc/net.cpp:293 in 5f3ba0e9ba outdated 
     289 | @@ -290,8 +290,7 @@ static RPCHelpMan addnode()
 290 |      if (!request.params[1].isNull())
 291 |          strCommand = request.params[1].get_str();
 292 |      if (strCommand != "onetry" && strCommand != "add" && strCommand != "remove") {
 293 | -        throw std::runtime_error(
 294 | -            self.ToString());
 295 | +        throw std::runtime_error(self.ToString());
    pablomartin4btc commented at 4:42 PM on August 12, 2022:

    Due to the amount of files changed and the context of this PR, perhaps we could leave this out(?) and follow it up in a different change.

  26. achow101 closed this on Oct 12, 2022
  27. achow101 reopened this on Oct 12, 2022
  28. achow101 commented at 7:24 PM on October 12, 2022: member

    Closed and reopened to hopefully trigger CI

  29. suhailsaqan commented at 9:41 PM on October 12, 2022: contributor

    @achow101 Any idea why CI is not running on this PR?

  30. fanquake commented at 2:58 AM on October 13, 2022: member

    achow101 Any idea why CI is not running on this PR?

    Rebasing may help. While you are at it, you can also squash your commits.

  31. maflcko commented at 8:43 AM on October 13, 2022: member

    Triggered CI

  32. suhailsaqan force-pushed on Oct 13, 2022
  33. DrahtBot added the label Needs rebase on Dec 8, 2022
  34. fanquake marked this as a draft on Feb 15, 2023
  35. 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.

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.

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
    72bd52a891
  36. keeps bitcoin-cli autocomplete in sync
    test: keeps bitcoin-cli autocomplete in sync

fixed linter error

added bash-completion to makefile

fixed typo

autogenerate bash completion

autogenerate bash completion

added tool_bash_completion.py to test_runner.py

added format RPC

fixed typo

added python functional test

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.

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

removed duplicates

removed unnecessary change
    c3d3f69e70
  37. added format to RPC_COMMANDS_NOT_SAFE_FOR_FUZZING f8858f7ad4
  38. suhailsaqan force-pushed on Feb 18, 2023
  39. remove unnecessary changes 3f06340ce2
  40. DrahtBot removed the label Needs rebase on Feb 18, 2023
  41. DrahtBot added the label Needs rebase on Jun 1, 2023
  42. DrahtBot commented at 8:10 PM on June 1, 2023: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

  43. DrahtBot commented at 1:45 AM on August 30, 2023: contributor

    <!--13523179cfe9479db18ec6c5d236f789-->

    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.
  44. achow101 commented at 4:56 PM on September 23, 2023: member

    Are you still working on this?

  45. DrahtBot commented at 12:57 AM on December 22, 2023: contributor

    <!--13523179cfe9479db18ec6c5d236f789-->

    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.
  46. maflcko closed this on Dec 22, 2023
  47. bitcoin locked this on Dec 21, 2024
Contributors
suhailsaqanfanquakelaanwjmaflckobrunoergluke-jrDrahtBothernanmarinopablomartin4btcachow101
Labels
RPC/REST/ZMQScripts and toolsNeeds rebase
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-29 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me