fix: Apps CLI parity #980
fix: Apps CLI parity #980
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 972746a in 37 seconds
More details
- Looked at
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/cli/apps.ts:48
- Draft comment:
Consider using a logging library instead of console.log for error messages to provide more context and better manage log levels. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_DqoJN7nFRtSLnz85
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
js/src/cli/apps.ts
Outdated
| getOpenAPIClient(); | ||
| const onlyShowEnabledApps = options.enabled; | ||
| console.log(onlyShowEnabledApps) |
There was a problem hiding this comment.
Remove this console.log statement as it seems to be for debugging purposes and is not necessary for production code.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ab4ee4b in 14 seconds
More details
- Looked at
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/cli/apps.ts:39
- Draft comment:
Consider using a logging library instead ofconsole.logfor error messages to allow better control over logging levels and outputs. - Reason this comment was not posted:
Confidence changes required:50%
The use of console.log for error handling is not ideal for production code. It would be better to use a logging library or mechanism that can be configured for different environments.
Workflow ID: wflow_wDY8CotDz3eUDrsI
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
js/src/cli/apps.ts
Outdated
| getOpenAPIClient(); | ||
| const onlyShowEnabledApps = options.enabled; | ||
| console.log(onlyShowEnabledApps) |
There was a problem hiding this comment.
This console.log statement appears to be a debugging leftover and should be removed from production code.
js/src/cli/apps.ts
Outdated
| const command = this.program.command("apps"); | ||
| const command = this.program.command("apps").option( | ||
| "--enabled", | ||
| "Only show enabled apps" |
There was a problem hiding this comment.
Consider adding a more descriptive help message for the --enabled flag, such as "Filter to show only enabled apps in the list"
Code Review SummaryThe changes look generally good with a few minor improvements needed: Positives:✅ The implementation of the --enabled flag is functionally correct Areas for Improvement:
Overall code quality: 7/10 - Functional but needs minor cleanup before merging. |
|
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: |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on cc60b51 in 11 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_WhwfjEB66k7qb34w
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Added the missing options for apps command in CLI
Important
Adds
--enabledoption toappscommand inAppsCommandto filter and display only enabled apps, with updated handling inhandleAction.--enabledoption toappscommand inAppsCommandto filter and display only enabled apps.handleActionmethod inAppsCommandto handleenabledoption and filter apps accordingly.enabledoption value inhandleAction.This description was created by
for cc60b51. It will automatically update as commits are pushed.