Skip to content

refactor: drop cli-table, payjoin history as JSON - #389

Merged
vadim-anfv merged 2 commits into
bitcoindevkit:masterfrom
vadim-anfv:ci/fix-opt-deps
Nov 14, 2026
Merged

vadim-anfv merged 2 commits into
bitcoindevkit:masterfrom
vadim-anfv:ci/fix-opt-deps

Conversation

@vadim-anfv

@vadim-anfv vadim-anfv commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Description

The --no-default-features build and clippy jobs are red on every open PR, including Markdown-only ones (#386, #387). cli-table, tracing and tracing-subscriber are unused there, and recent cargo flags that (unused_dependencies, warn by default) while the workflow passes build-warnings: deny.

cli-table is dropped entirely, as @tvpeter suggested: its last user, the payjoin session history, now returns JSON like the rest of the module.

payjoin history on master:

+------------+-----------------+---------------------+-------------------------------+
| Session ID | Sender/Receiver | Completed At        | Status                        |
+------------+-----------------+---------------------+-------------------------------+
| 1          | Receiver        | Not Completed       | Waiting for original proposal |
+------------+-----------------+---------------------+-------------------------------+
| 2          | Receiver        | 2026-11-13 17:50:42 | Session cancelled             |
+------------+-----------------+---------------------+-------------------------------+

and on this branch:

{
  "sessions": [
    {
      "id": "1",
      "role": "Receiver",
      "status": "Waiting for original proposal",
      "completed_at": null
    },
    {
      "id": "2",
      "role": "Receiver",
      "status": "Session cancelled",
      "completed_at": "2026-11-13 18:34:22"
    }
  ]
}

Notes to the reviewers

An active session now reports "completed_at": null instead of the string Not Completed.

tracing is on cbf explicitly, although cbf already implies _payjoin-dependencies, so cbf logging does not depend on a payjoin-internal feature.

just pre-push misses this: the lint is cargo's, not rustc's, so -D warnings after -- does not apply. To reproduce: cargo build --no-default-features --config 'build.warnings="deny"'.

The SessionHistory wrapper exists to keep the field order: serializing it directly preserves declaration order, while json! would sort the keys alphabetically.

Checklists

All Submissions:

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

@codecov

codecov Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.33%. Comparing base (12aae53) to head (eb9536d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #389      +/-   ##
==========================================
- Coverage   59.50%   59.33%   -0.17%     
==========================================
  Files          25       25              
  Lines        4583     4562      -21     
==========================================
- Hits         2727     2707      -20     
+ Misses       1856     1855       -1     
Flag Coverage Δ
rust 59.33% <100.00%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tvpeter tvpeter left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you @vadim-anfv for checking the CI failures.

I think we should get ride of cli-table since the pretty flag was dropped.

Comment thread Cargo.toml Resolved
@tvpeter tvpeter added this to the CLI 4.1.0 milestone Nov 13, 2026
@vadim-anfv vadim-anfv changed the title ci: make cli-table and tracing optional ci: drop cli-table, make tracing optional Nov 13, 2026
@vadim-anfv
vadim-anfv requested a review from tvpeter November 13, 2026 16:38
@tvpeter

tvpeter commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator

I just merged so you have conflicts. Also, update the PR title too. Thank you.

The last cli-table user, dead weight since --pretty was dropped in bitcoindevkit#278.
Unused in a --no-default-features build, which recent cargo warns about
and CI denies. cli-table has no users left at all.
@vadim-anfv vadim-anfv changed the title ci: drop cli-table, make tracing optional refactor: drop cli-table, payjoin history as JSON Nov 13, 2026
@vadim-anfv

Copy link
Copy Markdown
Collaborator Author

I just merged so you have conflicts. Also, update the PR title too. Thank you.

Rebased onto master, title updated, please have a look.

@tvpeter tvpeter left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ACK eb9536d

Thank you.

@vadim-anfv
vadim-anfv merged commit ea06679 into bitcoindevkit:master Nov 14, 2026
10 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in BDK-CLI Nov 14, 2026
@vadim-anfv
vadim-anfv deleted the ci/fix-opt-deps branch November 14, 2026 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants