Skip to content

Fix exit code #173

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

Merged
merged 1 commit into from
Jun 3, 2025
Merged

Fix exit code #173

merged 1 commit into from
Jun 3, 2025

Conversation

va-an
Copy link
Contributor

@va-an va-an commented Jan 21, 2025

Fix for #170.

I think this is the simplest way to do it.
It's also possible to change the main signature to return Result, which should provide similar behavior, but this option isn't compatible with the #[maybe_async] macro, so I decided not to complicate things.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

Sorry, something went wrong.

@tvpeter
Copy link
Collaborator

tvpeter commented Apr 15, 2025

@va-an, could you please rebase and confirm if the issue still exists; otherwise close the PR. Thank you for your efforts.

@va-an
Copy link
Contributor Author

va-an commented Apr 17, 2025

@va-an, could you please rebase and confirm if the issue still exists; otherwise close the PR. Thank you for your efforts.

I've replied in #170.
Let's discuss there.

@va-an
Copy link
Contributor Author

va-an commented Apr 18, 2025

@tvpeter, rebased to master.
I composed a command as close as possible to the one in the original issue in order to reproduce the behavior.

From master - exit code 0:

-> % cargo run --features electrum -- \
    --network signet \
    wallet \
    --int-descriptor "" \
    --client-type electrum \
    --database-type sqlite \
    --url some \
    new_address
[2025-04-18T14:31:56Z ERROR bdk_cli] Generic error: An external descriptor is required.
-> % echo $?
0

With fix - exit code 1:

-> % cargo run --features electrum -- \
    --network signet \
    wallet \
    --int-descriptor "" \
    --client-type electrum \
    --database-type sqlite \
    --url some \
    new_address
[2025-04-18T14:31:19Z ERROR bdk_cli] Generic error: An external descriptor is required.
-> % echo $?
1

@notmandatory notmandatory added the bug Something isn't working label Apr 30, 2025
@va-an
Copy link
Contributor Author

va-an commented May 17, 2025

Please let me know what I can do to make this PR ready to merge.
In the meantime, I'll continue studying the bdk source code to be able to make more significant changes in the future.

@va-an
Copy link
Contributor Author

va-an commented May 19, 2025

Made a force-push to sign my commit.

@notmandatory notmandatory moved this to Ready to Review in BDK-CLI May 28, 2025
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 04c7f88

@notmandatory notmandatory added this to the CLI 1.1.0 milestone May 28, 2025
@tvpeter
Copy link
Collaborator

tvpeter commented May 28, 2025

@va-an please rebase.

Verified

This commit was signed with the committer’s verified signature.
@va-an
Copy link
Contributor Author

va-an commented Jun 2, 2025

@va-an please rebase.

Done! I also added a checklist to the description.

@tvpeter tvpeter merged commit f56ca5e into bitcoindevkit:master Jun 3, 2025
8 checks passed
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in BDK-CLI Jun 3, 2025
@coveralls
Copy link

Pull Request Test Coverage Report for Build 15389199914

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 2.7%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/main.rs 0 2 0.0%
Totals Coverage Status
Change from base Build 15127343813: -0.003%
Covered Lines: 25
Relevant Lines: 926

💛 - Coveralls

@va-an va-an deleted the fix_exit_code branch June 3, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants