Add cli build command - #217
Conversation
JCZuurmond
left a comment
JCZuurmond
left a comment
There was a problem hiding this comment.
LGTM. Added some clean up comments.
Main point to change is to keep the __main__.py. I recommend to rename cli.py to __main__.py.
Approving so you can move forward
| @@ -1,4 +0,0 @@ | |||
| from .cli import app | |||
There was a problem hiding this comment.
Please keep the main, though, you can move it to the top-level. This allows to run the cli as: python -m pg2_dataset
| """ | ||
|
|
||
| if version: | ||
| typer.echo(f"v{__version__}") |
There was a problem hiding this comment.
The docs suggest to use rich for prettier console logging: https://typer.tiangolo.com/tutorial/printing/#printing-and-colors.
Otherwise, I prefer plain Python logging.
| typer.echo(f"v{__version__}") | ||
| raise typer.Exit() | ||
|
|
||
| if ctx.invoked_subcommand is None: |
There was a problem hiding this comment.
Ha, I like the explicit is None, but a "Pythonic" approach is:
| if ctx.invoked_subcommand is None: | |
| if not ctx.invoked_subcommand: |
Does not matter IMO if we are consistent across the code base(s)
| Returns: | ||
| None | ||
|
|
There was a problem hiding this comment.
| Returns: | |
| None |
| """Creates a Dataset instance from a manifest TOML file and dumps it as zip to a | ||
| specified directory path. |
There was a problem hiding this comment.
First sentence should fit on one line. Also the implementation details can be hidden here, write the intent instead or add the implementation details in a first paragraph:
| """Creates a Dataset instance from a manifest TOML file and dumps it as zip to a | |
| specified directory path. | |
| """Build a protein gym dataset. | |
| <implementation details> |
| manifest_path: The path to the manifest TOML file. | ||
| output_path: The directory path to dump the dataset archive. |
There was a problem hiding this comment.
| manifest_path: The path to the manifest TOML file. | |
| output_path: The directory path to dump the dataset archive. | |
| manifest_path (Path) : The path to the manifest TOML file. | |
| output_path (Path | None) : The directory path to dump the dataset archive. If `None`, the current working directory is used. Defaults to `None`. |
|
|
||
| Returns: None |
There was a problem hiding this comment.
| Returns: None |
|
|
||
| Args: | ||
| ctx: The context for the CLI. | ||
| version: If True, show the version and exit. |
There was a problem hiding this comment.
| version: If True, show the version and exit. | |
| version (bool): If `True`, show the package version. Defaults to `False`. |
| """Main entry point for the CLI. | ||
|
|
||
| Args: | ||
| ctx: The context for the CLI. |
There was a problem hiding this comment.
| ctx: The context for the CLI. | |
| ctx (type.Context): The context for the CLI. |
| assert result.exit_code == 0 | ||
| assert result.stdout.startswith("v") | ||
| assert __version__ in result.stdout | ||
|
|
2 participants
Changes
Resolves #190
This PR adds a build command to the cli.
Checklist