-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix: Added missing params for add cli command #977
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 2e4b647 in 1 minute and 59 seconds
More details
- Looked at
346
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. js/src/cli/add.ts:130
- Draft comment:
Consider defining a specific type for theoptions
parameter instead of usingany
to improve type safety and code readability. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. js/src/cli/add.ts:176
- Draft comment:
Consider defining a specific type for theoptions
parameter instead of usingany
to improve type safety and code readability. - Reason this comment was not posted:
Marked as duplicate.
3. js/src/cli/add.ts:97
- Draft comment:
Avoid usingprocess.exit(0)
directly. Consider returning a value or throwing an error and handling the exit at a higher level for better testability and maintainability. - Reason this comment was not posted:
Comment was on unchanged code.
4. js/src/cli/add.ts:161
- Draft comment:
Avoid usingprocess.exit(0)
directly. Consider returning a value or throwing an error and handling the exit at a higher level for better testability and maintainability. - Reason this comment was not posted:
Marked as duplicate.
5. js/src/cli/add.ts:172
- Draft comment:
Avoid usingprocess.exit(0)
directly. Consider returning a value or throwing an error and handling the exit at a higher level for better testability and maintainability. - Reason this comment was not posted:
Marked as duplicate.
6. js/src/cli/add.ts:184
- Draft comment:
Avoid usingprocess.exit(0)
directly. Consider returning a value or throwing an error and handling the exit at a higher level for better testability and maintainability. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_tdyripdUJhqCNzcx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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! Incremental review on 1de5933 in 33 seconds
More details
- Looked at
362
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. js/src/cli/add.ts:219
- Draft comment:
Consider providing a default value foroptions
to avoid potential runtime errors when accessing its properties.
options: any = {}
- Reason this comment was not posted:
Comment was on unchanged code.
2. js/src/cli/add.ts:298
- Draft comment:
Consider refactoringhandleBasicAuth
andhandleOAuth
to reduce code duplication, especially in how they callsetupIntegration
. - Reason this comment was not posted:
Confidence changes required:50%
ThehandleBasicAuth
andhandleOAuth
methods are very similar in structure, especially in how they callsetupIntegration
. This could be refactored to reduce code duplication.
3. js/src/cli/add.ts:376
- Draft comment:
Consider unifyingdisplayName
anddisplay_name
to a single naming convention to avoid confusion and potential errors. - Reason this comment was not posted:
Confidence changes required:50%
ThecollectInputFields
function uses bothdisplayName
anddisplay_name
for field names. This could be unified to avoid confusion and potential errors.
Workflow ID: wflow_7uRh75bmPjixy0MQ
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
Enhances
add
CLI command with new options and refactors integration and connection handling inadd.ts
.-n, --no-browser
to prevent browser opening for connection verification.-i, --integration-id <id>
to specify an existing integration ID.-a, --auth-mode <mode>
to specify authentication mode.-s, --scope <scope>
to specify connection scopes.-l, --label <label>
for connected account labels.handleAction()
to useintegrationId
if provided, otherwise list integrations.createIntegration()
to handle different auth modes and schemes.handleBasicAuth()
andhandleOAuth()
for specific auth handling.setupConnections()
to handlescope
andlabel
options.shouldForceConnectionSetup()
.This description was created by for 1de5933. It will automatically update as commits are pushed.