Refactor cleanup to use RecipeBuilder methods #260
Conversation
Let's keep it as-is for now and see what we would want to remove and want we want to keep. We disabled those parameters in user facing CLI, so we can make non-breaking-changes anytime. |
| build_dir_base | ||
| Optional root path under which package build directories live: | ||
| <build_dir_base>/<package>/build. If provided, overrides the default | ||
| per-package location, but can still be overridden directly with build_dir. |
There was a problem hiding this comment.
Could you please explain why we need this extra parameter? build_dir should be enough to set the build directory I guess.
| Path(recipe_dir).expanduser().resolve() if recipe_dir else (root / "packages") | ||
| Path(recipe_dir).expanduser().resolve() | ||
| if recipe_dir | ||
| else RecipeBuilder.get_default_recipe_dir() |
There was a problem hiding this comment.
I prefer not to add this staticmethod just to call it in our CLI. The recipe builder should take the minimum information as a parameter in its constructor, then calculate and store the information internally.
Unfortunately, our existing CLI might not be implemented in that way (we are passing a bunch of information in build-recipes CLI as well), but I don't think this approach helps mitigate it.
Instead, maybe let's calculate only the recipe directory as-is in the orignal code (using search_pyodide_root directly), and pass other information as None. Then the RecipeBuilder should be responsible for calculating those information internally.
| self.force_rebuild = force_rebuild or continue_ | ||
| self.continue_ = continue_ | ||
|
|
||
| def cleanup_paths(self, *, include_dist: bool = False) -> list[Path]: |
There was a problem hiding this comment.
| def cleanup_paths(self, *, include_dist: bool = False) -> list[Path]: | |
| def _cleanup_paths(self, *, include_dist: bool = False) -> list[Path]: |
Let's prefix with underscore as it is used internally only.
| for path in self.cleanup_paths(include_dist=include_dist): | ||
| try: | ||
| if path.is_dir(): | ||
| if path.exists(): |
There was a problem hiding this comment.
| if path.exists(): |
is_dir implies that the path exists.
| if include_dist and install_dir is not None: | ||
| _remove_path(install_dir) |
There was a problem hiding this comment.
Let's remove these for now as we are not using it in our CLI. I don't want to keep the same function both here and in the recipe builder.
2 participants
Related to #218. Follow-up to #254.
Refactors cleanup logic to use
RecipeBuildermethods.As mentioned in #223 (comment), I agree that using
--allinstead of--include-distwould be more intuitive. I can include that change in this PR if that's acceptable.