Skip to content

refactor(cli): simplify bip cli args#1554

Merged
arminsabouri merged 1 commit into
payjoin:masterfrom
va-an:refactor/simplify-cli-args
May 13, 2026
Merged

refactor(cli): simplify bip cli args#1554
arminsabouri merged 1 commit into
payjoin:masterfrom
va-an:refactor/simplify-cli-args

Conversation

@va-an
Copy link
Copy Markdown
Contributor

@va-an va-an commented May 12, 2026

Closes #1530

Simplify --bip77/--bip78 args, drop redundant clap attributes.
The cli api didn't change, just internal refactoring.

I used Claude Code for research, but wrote all changes manually.

Pull Request Checklist

Please confirm the following before requesting review:

@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented May 12, 2026 •
edited
Loading

Coverage Report for CI Build 25796631085

Coverage remained the same at 85.294%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: 4 of 4 lines across 2 files are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats

Coverage Status
Relevant Lines: 13668
Covered Lines: 11658
Line Coverage: 85.29%
Coverage Strength: 395.79 hits per line
💛 - Coveralls

@va-an va-an force-pushed the refactor/simplify-cli-args branch from edec5fb to 0ba27ca Compare May 12, 2026 20:44
@zealsham
Copy link
Copy Markdown
Collaborator

zealsham commented May 13, 2026

tACK

can you squash your commits in other to follow the contribution guidelines

@va-an va-an force-pushed the refactor/simplify-cli-args branch from a803aec to a7d5ecb Compare May 13, 2026 10:02
@va-an
Copy link
Copy Markdown
Contributor Author

va-an commented May 13, 2026

tACK

can you squash your commits in other to follow the contribution guidelines

I wanted to keep each change atomic.
Squashed the 3 commits related to struct Flags into one.

@zealsham
Copy link
Copy Markdown
Collaborator

zealsham commented May 13, 2026

tACK
can you squash your commits in other to follow the contribution guidelines

I wanted to keep each change atomic. Squashed the 3 commits related to struct Flags into one.

This is such a small change that it doesn't apply, atomicity here is more of "a pr should do only one thing and not multiple unrelated things "

also even for bigger changes. from our merge history, you can see that it eventually get squashed into one commit before merge

@va-an
refactor(cli): simplify struct Flags
7aa7e48 
- `Option<bool>` -> `bool`. with `ArgAction::SetTrue` the flag
  is `true` when present and `false` when absent. `None` was
  a state the parser could never produce.
- drop `action = clap::ArgAction::SetTrue`. clap default to
  `SetTrue` for a plain `bool` field.
- drop `long = "bipNN"`. clap derive it from the field name.
- drop `Clone` and `Deserialize`. both were dead code: `Flags`
  is never cloned or deserialized.

Also changed:
- update payjoin-cli README
- remove rustdoc on `BitcoindWallet` mentioning a non-existent
    `PayjoinWallet` trait
@va-an va-an force-pushed the refactor/simplify-cli-args branch from a7d5ecb to 7aa7e48 Compare May 13, 2026 11:36
@va-an
Copy link
Copy Markdown
Contributor Author

va-an commented May 13, 2026

This is such a small change that it doesn't apply, atomicity here is more of "a pr should do only one thing and not multiple unrelated things "

also even for bigger changes. from our merge history, you can see that it eventually get squashed into one commit before merge

Got it.
Squashed into one commit, pls have a look.

@zealsham
Copy link
Copy Markdown
Collaborator

zealsham commented May 13, 2026

i think this is ready to get merged

@nothingmuch
Copy link
Copy Markdown
Contributor

nothingmuch commented May 13, 2026

can you squash your commits in other to follow the contribution guidelines

wait, where does it say this? it mentions hygienic commits following bitcoin core's reccomendations, which usually means to not squash when changes are not related

nothingmuch
nothingmuch approved these changes May 13, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@nothingmuch nothingmuch left a comment

Choose a reason for hiding this comment

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

utACK

@arminsabouri
Copy link
Copy Markdown
Collaborator

arminsabouri commented May 13, 2026

This is such a small change that it doesn't apply, atomicity here is more of "a pr should do only one thing and not multiple unrelated things "

For future reference the size of the change doesnt matter. if two changes are unrelated they should be in seperate commits

arminsabouri
arminsabouri approved these changes May 13, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@arminsabouri arminsabouri left a comment

Choose a reason for hiding this comment

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

Ack 7aa7e48

@arminsabouri arminsabouri merged commit 7aba9e8 into payjoin:master May 13, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arminsabouri arminsabouri arminsabouri approved these changes

+1 more reviewer

@nothingmuch nothingmuch nothingmuch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cli: simplify Flags fields from Option<bool> to bool

5 participants

@va-an @coveralls @zealsham @nothingmuch @arminsabouri