-
Notifications
You must be signed in to change notification settings - Fork 69
Add Prompt Library support #1168
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
Conversation
The following subcommands are included: - `list`: lists prompts - `get`: gets a prompt by ID - `create`: creates a prompt - `update`: updates a prompt - `delete`: deletes a prompt - `tags`: manages prompt tags (with subcommands: list, create, update, delete)
This commit introduces enhancements to the prompt management commands, allowing users to select specific columns for display and output results in JSON format. The following changes were made: - Added column selection functionality to `src prompts list` and `src prompts tags list` commands using the `-c` flag. Users can now specify a comma-separated list of columns to display. - Implemented JSON output functionality for `src prompts list` and `src prompts tags list` commands using the `-json` flag. - Updated `src prompts update`, `src prompts tags update`, `src prompts delete`, and `src prompts tags delete` commands to accept the ID as a positional argument instead of a flag. - Updated `src prompts export` to fetch all tags with pagination.
This commit modifies the `src prompts create` command to default the prompt owner to the current user if no owner is explicitly specified via the `-owner` flag.
documentation updates are in
sourcegraph/docs#1155
@trly Heya, thanks for building this! Before heading into the review I wanted to ask if this has been mainly Amp generated, and if you've done a cleanup pass after letting Amp do its work.
Just pushed a formatting fix, but other than than, it all appears to be structured very similar to the search-jobs functionality that I recently added without Amp and used as a reference for myself and Amp when adding this feature.
And, yes it was mainly Amp generated, but throughly reviewed in comparison to work I had previously done on this repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about making the 100 configurable via a flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been back and forth on that a few times. I really don't know how well the CLI is going to display a large number of tags anyway.
Maybe add a flag but set the default lower (10)?
Add documentation for the `src prompts` command and its subcommands. Added documentation for the new src prompts commands sourcegraph/src-cli#1168 ## Pull Request approval You will need to get your PR approved by at least one member of the Sourcegraph team. For reviews of docs formatting, styles, and component usage, please tag the docs team via the #docs Slack channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always error with the format shown in the example. When <prompt-id> is the first argument, none of the flags are parsed as flags, but are instead additional arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All fields are required? Even if updating only one field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: get for other entities uses the ID, not the name/value. Also, tags delete and tags update take the ID as input. This breaks that pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments about inconsistencies and small improvements, but overall it works. Worth a followup PR or two to polish it.
Test plan
Changes were manually tested: https://ampcode.com/threads/T-d71803f6-01cb-4655-8150-95a9f4517899
✅ Basic Commands: Help, command structure, and navigation
✅ Tag CRUD: Create, read, update, delete operations for prompt tags
✅ Prompt CRUD: Create, read, update, delete operations for prompts
✅ Export/Import: Full workflow including data integrity verification
✅ Advanced Filtering: Owner, affiliated, built-in, recommended, draft filtering
✅ Search & Pagination: Query search, column selection, cursor pagination
✅ Long Content: Handles large prompt content (2.7KB+ tested)
✅ Unicode Support: Full Unicode in descriptions/content, restricted in names
✅ Empty Libraries: Graceful handling of empty prompt collections
✅ Malformed Data: Robust JSON validation and error reporting
✅ Character Validation: Database constraints properly enforced