Refactor: factorise plugins app implementation #1166
Conversation
| @@ -55,7 +64,7 @@ def _mask_password(connection_params: dict): | |||
| return connection_params | |||
|
|
|||
|
|
|||
| @app.command(name="list") | |||
| @app_creator.command(name="list") | |||
There was a problem hiding this comment.
Using app will reduce a lot of changes, I don't think we need to to the rename
| class ConnectionAppCreator(SnowTyperCreator): | ||
| def create_app(self) -> SnowTyper: | ||
| app = SnowTyper( | ||
| name="connection", | ||
| help="Manages connections to Snowflake.", | ||
| ) | ||
| self.register_commands(app) | ||
| return app |
There was a problem hiding this comment.
Can't we simplify it to something like
app: SnowTyperCreator = SnowTyperCreator.create_app(SnowTyper(
name="connection",
help="Manages connections to Snowflake.",
))do we really need a custom class for every command group?
There was a problem hiding this comment.
I could pass **kwargs from create_app to the constructor, but then I'd have to modify add_init_command-like procedures to work with SnowTyperCreator - I will check
There was a problem hiding this comment.
changed as discussed
| def __init__(self): | ||
| self.commands_to_register: List[SnowTyperCommandData] = [] | ||
|
|
||
| def create_app(self) -> SnowTyper: | ||
| """ | ||
| Returns SnowTyper instance. | ||
| """ | ||
| raise NotImplementedError() | ||
|
|
||
| def command(self, *args, **kwargs): | ||
| def decorator(command): | ||
| self.commands_to_register.append( | ||
| SnowTyperCommandData(command, args=args, kwargs=kwargs) | ||
| ) | ||
| return command | ||
|
|
||
| return decorator | ||
|
|
||
| def register_commands(self, app: SnowTyper) -> None: | ||
| for command in self.commands_to_register: | ||
| app.command(*command.args, **command.kwargs)(command.func) |
There was a problem hiding this comment.
Why we cannot add this logic to SnowTyper? What's reasoning behind adding this additional layer?
There was a problem hiding this comment.
We want to postpone calling __init__ until error handling would be set up. The problem behind the issue was calling feature flags (which read config.toml) to determine constructor arguments (hidden=FeatureFlag.is_disabled)
There was a problem hiding this comment.
changed as discussed
sfc-gh-pczajka
changed the title
| kwargs: Dict[str, Any] | ||
|
|
||
|
|
||
| class SnowTyper: |
There was a problem hiding this comment.
Should we name it SnowTyperFactory?
There was a problem hiding this comment.
renamed:
SnowTyper -> SnowTyperFactory
SnowTyperInstance -> SnowTyper
.create_app() -> .create_instance()
There was a problem hiding this comment.
- src/snowflake/cli/plugins/nativeapp/version/commands.py also needs these changes.
There was a problem hiding this comment.
It is already changed :)
sfc-gh-pczajka
requested review from
sfc-gh-pjob,
sfc-gh-turbaszek and
sfc-gh-bgoel
| ) | ||
|
|
||
| app.add_typer(stage_app) # type: ignore | ||
| app.add_typer(show_app) # type: ignore |
There was a problem hiding this comment.
I made it consistent with other plugins (spcs / NADE), but I made the choice between __init__.py and commands.py arbitrarily - no strong opinion
| ╰──────────────────────────────────────────────────────────────────────────────╯ | ||
|
|
||
| ''' | ||
| # --- |
sfc-gh-pczajka
dismissed stale reviews from sfc-gh-bgoel and sfc-gh-turbaszek
via
f1027e8
Pre-review checklist
Changes description
Refactor: change SnowTyper into Typer instance fanctory.
Only developer-facing change: we need to call
SnowTyper.create_app()to get an actual Typer instance.The motivation behind the change is an issue #813, which was caused by reading
config.tomlbefore error handling was set up