Add CLI command pyodide clean recipes#254
Conversation
ryanking13
left a comment
There was a problem hiding this comment.
Thanks for working on this @teddygood! I left some comments
pyodide_build/cli/clean.py
Outdated
|
|
||
|
|
||
| @app.command("recipes") | ||
| def clean_recipes( # noqa: D401 - Typer generates help text. |
There was a problem hiding this comment.
Why is D401 ignored? What does it complain about?
There was a problem hiding this comment.
I was debating whether to include it in the PR before submitting, but after reading the pyproject.toml just now, I found that the docstring style check isn’t enabled... I'll remove it. Thanks for the review.
pyodide_build/cli/clean.py
Outdated
|
|
||
| from pyodide_build import build_env | ||
| from pyodide_build.logger import logger | ||
| from pyodide_build.recipe.cleanup import clean_recipes as perform_cleanup |
There was a problem hiding this comment.
How about importing recipe and calling recipe.clean_recipes()? This alias looks a bit redundant.
There was a problem hiding this comment.
I agree, that looks better. I'll fix it.
| return | ||
|
|
||
|
|
||
| def _resolve_paths( |
There was a problem hiding this comment.
(No action required for this PR)
We may improve this as the default values are hardcoded here and there, not organized. As you mentioned in the next steps, we can probably handle this under the recipe builder class.
There was a problem hiding this comment.
I hadn't thought about that. Thanks for pointing it out. I'll make sure to address it in a future refactor.
pyodide_build/cli/clean.py
Outdated
| include_dist: bool = typer.Option( | ||
| False, | ||
| help="Remove per-package and global dist directories.", | ||
| ), | ||
| include_always_tag: bool = typer.Option( | ||
| False, | ||
| help="Include packages tagged with 'always' when no explicit targets are provided.", | ||
| ), |
There was a problem hiding this comment.
I think providing these parameters to users might not be that useful (sorry that I didn't comment it in the previous PR)
- I think people would not want to remove the
distdirectory mostly. - Also cleaning up the
alwayspackages when it is not specified as a target doesn't seem intuitive either.
So maybe let's remove these parameters for now (install_dir will not be needed either in that case), and set include_dist=False, include_always_tag=False.
There was a problem hiding this comment.
I’ll remove those options and keep the default values set to False.
ryanking13
left a comment
There was a problem hiding this comment.
Thanks!
Could you also update the CHANGLEOG?
pyodide_build/cli/clean.py
Outdated
| """ | ||
| Remove build artifacts for recipe packages. | ||
| """ | ||
| recipe_path, build_path, install_path = _resolve_paths( |
There was a problem hiding this comment.
install_path is not used anymore, so it can removed from _resolve_paths's return value (and also from the parameter).
There was a problem hiding this comment.
Thanks for letting me know.
Ok, I’ll check the previous commits and update it. |
e6064b3 to
6b7dcc8
Compare
Related to #218. Follow-up to #223.
This PR adds a new CLI command
pyodide clean recipes, which makes theclean_recipeshelper from the previous PR available through the main Pyodide CLI.pyodide clean, and defined the subcommandpyodide clean recipes.--include-distand--include-always-tagflags directly toclean_recipes.pyproject.tomlso it’s accessible viapyodide clean recipes.--include-distNext Steps (planned follow-ups)
These will be handled in separate PRs.
Please let me know if there’s anything I could add or improve in this PR.
pyodide cleancommand to support additional subcommands (e.g.,pyodide clean build,pyodide clean cache).RecipeBuilderto handle cleanup internally, adjust flag names (--include-dist->--all), and update related documentation.