Skip to content

Add delete publish#4

Merged
SpontaneousOverthrow merged 6 commits intomainfrom
feat/add_delete_publish
Oct 22, 2025
Merged

Add delete publish#4
SpontaneousOverthrow merged 6 commits intomainfrom
feat/add_delete_publish

Conversation

@SpontaneousOverthrow
Copy link
Contributor

No description provided.

README.md Outdated
Comment on lines 66 to 70
# 1) Get data by path
PUBKY_CLI_RECOVERY_PASSPHRASE=pass \
cargo run -- user get /pub/my-cool-app/hello.txt ./alice.recovery

# 2) Publish data from file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: but maybe publish/get/delete would be more natural order of operations

@piotr-iohk
Copy link
Collaborator

Some review feedback from Codex:

🍒 Key feedback

Repeated sign‑in / sign‑out pattern isn’t resilience-friendly

In all three helpers (publish_data, delete_data, get_data) the session is torn down only on the “happy path”. If storage.put/get/delete throws, you’ll skip the signout() call. Consider wrapping the signout in a match/finally block or call it in a drop guard so the session always gets closed.

Path validation

storage.put/delete/get expect an absolute Pubky path. Right now the CLI just forwards the string verbatim. It’d be safer to run it through PubkyResource::parse / ResourcePath::parse (or at least validate the /pub/... prefix) so users get a nice error rather than the SDK error.

get assumes UTF-8 text

.text() forces UTF-8; any binary file (even Lightning backups) will panic. You’ll probably want to stream bytes and either dump stdout as-is or add --outfile. Until then, document that this command is text-only.

CLI UX details

  • The argument descriptions say “Path to file /pub/test.txt” but don’t mention the requirement for absolute paths.
  • For publish, we probably want a --mime or at least to accept stdin so users don’t need a temp file (optional but worth thinking about).
  • get currently prints the fetched data alongside session diagnostics, which means piping to another command will pick up those logs. Maybe gate the diagnostic prints behind a --verbose.
  • Overall the feature is a good addition; just tighten up the validation, cleanup behavior, and test coverage. The binary’s new functionality will then be much safer to rely on.

@SpontaneousOverthrow SpontaneousOverthrow self-assigned this Oct 22, 2025
@SpontaneousOverthrow
Copy link
Contributor Author

I guess that's fine for the MVP

@SpontaneousOverthrow
Copy link
Contributor Author

we can also move the session creation/destroy to separate functions, but this is for refactor.

@SpontaneousOverthrow SpontaneousOverthrow merged commit d7135ac into main Oct 22, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants