Skip to content
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

feat: add cli option to read note from stdin #557

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lippoliv
Copy link

@lippoliv lippoliv commented Oct 17, 2024

This PR adds the CLI option --stdin that works like it is implemented in the add command: Instead of from --note <NOTE> the note's content is read from stdin.

This way something like cat some-long-note.txt | hoarder bookmarks update --stdin <ID> can work

like in "add" command

Signed-off-by: Oliver Lippert <[email protected]>
Copy link
Collaborator

@MohamedBassem MohamedBassem left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I think this makes sense. Left a couple of small comments.

@@ -187,13 +187,19 @@ bookmarkCmd
.description("update a bookmark")
.option("--title <title>", "if set, the bookmark's title will be updated")
.option("--note <note>", "if set, the bookmark's note will be updated")
.option("--stdin", "if set, the bookmark's note will be updated by stdin (not to be used with --note)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we give the flag a better name to indicate that it's updating the notes? Maybe --note-stdin or something?

Copy link
Author

Choose a reason for hiding this comment

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

Indeed I wondered about --stdin for add parameter as well. It's unclear what it's for. But I decided to be consistent. I'ld suggest to rename --stdin for both commands after the merge? Or rename it in parallel and I'll update + adapbt before merging this PR?

apps/cli/src/commands/bookmarks.ts Show resolved Hide resolved
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