-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: add auth_options decorator for CLI commands #57
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
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.
Important
Looks good to me! 👍
Reviewed 73bbbb3 in 39 seconds. Click for details.
- Reviewed
19lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/cli/decorators.py:51
- Draft comment:
Extra blank line after the docstring. Consider removing it for consistent formatting. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/datapilot/cli/decorators.py:87
- Draft comment:
Trailing whitespace removal appears consistent; ensure your formatter settings are maintained. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_IXSo56Ai3yyUAEOP
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed everything up to 16c1191 in 2 minutes and 21 seconds. Click for details.
- Reviewed
298lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/cli/decorators.py:56
- Draft comment:
The wrapper assumes that the first parameter is the Click context (ctx). Ensure all commands using this decorator include @click.pass_context and document this requirement. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/datapilot/core/platforms/dbt/cli/cli.py:159
- Draft comment:
Prompting for token and instance name within the command can block non‐interactive runs. Consider allowing non-interactive mode or validating input before prompting. - Reason this comment was not posted:
Comment was on unchanged code.
3. src/datapilot/cli/decorators.py:42
- Draft comment:
The process_config function performs only top‐level substitution of environment variables. If nested config values are expected, consider a recursive approach. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_arSM48XbuHu0MEkl
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
src/datapilot/cli/decorators.py
Outdated
| # Set defaults if nothing was provided | ||
| ctx.obj.setdefault("token", None) | ||
| ctx.obj.setdefault("instance_name", None) | ||
| ctx.obj.setdefault("backend_url", "https://api.myaltimate.com") |
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.
Avoid using a magic string for the backend URL default. Consider defining a constant to improve maintainability.
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.
Important
Looks good to me! 👍
Reviewed 34a829c in 23 seconds. Click for details.
- Reviewed
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/cli/decorators.py:62
- Draft comment:
Removed unused 'config_mapping' variable; cleanup improves clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_JX07SOD8T1NT3exw
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 6b4334b in 2 minutes and 2 seconds. Click for details.
- Reviewed
154lines of code in4files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/cli/decorators.py:82
- Draft comment:
Redundant default assignment: Lines setting final_token/instance_name to None are unnecessary since they already hold None if not provided. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/datapilot/cli/decorators.py:74
- Draft comment:
Consider using a truthiness check instead of explicit 'is None' for CLI parameters. An empty string ("") would bypass the file config merge even if it should be considered absent. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/datapilot/cli/main.py:11
- Draft comment:
Removal of click.pass_context simplifies the command signature; ensure that no essential context data is lost. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/datapilot/core/knowledge/cli.py:20
- Draft comment:
Replacing ctx.exit(1) with raise click.Abort() is acceptable; ensure that the behavior (exit code and error messaging) is as intended. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. src/datapilot/core/platforms/dbt/cli/cli.py:153
- Draft comment:
For the onboard command, when prompting for the API Token, consider adding hide_input=True to prevent echoing sensitive input. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_98rzUAe9fops6LiM
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed ee8d1a5 in 1 minute and 8 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_5aWxQ539f0sHBnxW
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed ace1521 in 44 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/knowledge/cli.py:24
- Draft comment:
Raising click.Abort is more explicit than returning, ensuring a non-zero exit. Confirm this is the intended behavior for signaling an error. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_lKyMmA5CUnB5sLaC
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Refactor CLI commands to use a new
auth_optionsdecorator for centralized authentication handling, simplifying code and ensuring consistent behavior across commands.auth_optionsdecorator indecorators.pyto handle authentication options for CLI commands.~/.altimate/altimate.jsonand substitutes environment variables.auth_optionstoserve()inknowledge/cli.pyandproject_health()andonboard()indbt/cli.py.main.pyand other CLI functions.This description was created by
for ace1521. You can customize this summary. It will automatically update as commits are pushed.