-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 trigger list command #800
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hey @himanshu-dixit, here is an example of how you can ask me to improve this pull request: @Sweep Add unit tests for the new `list` command in `triggers.ts` that verify: 📖 For more information on how to use Sweep, please read our documentation. |
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.
👍 Looks good to me! Reviewed everything up to 46476b6 in 16 seconds
More details
- Looked at
29
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. js/src/cli/triggers.ts:35
- Draft comment:
The 'list' command should be registered as a subcommand of 'triggers'. Consider usingcommand.command('triggers list')
instead ofcommand.command('list')
. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The current implementation of the 'list' command as a subcommand of 'triggers' seems correct. The suggestion to usecommand.command('triggers list')
is unnecessary because the 'triggers' command is already established as the parent. The comment does not provide strong evidence that a change is required.
I might be missing some context about how the command structure is intended to be used, but based on the provided code, the current structure seems logical and consistent.
The code clearly shows that 'list' is a subcommand of 'triggers', and the suggestion in the comment does not align with the existing structure. The comment lacks strong evidence for a necessary change.
The comment should be deleted as it does not provide strong evidence for a necessary code change and the current implementation appears correct.
Workflow ID: wflow_WWWWGrru1VkOVSik
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
Important
Add
list
command toConnectionsCommand
intriggers.ts
to list all triggers and update version inpackage.json
.list
command toConnectionsCommand
intriggers.ts
to list all triggers.package.json
from0.2.7
to0.2.8
.This description was created by for 46476b6. It will automatically update as commits are pushed.