feat: add cleanup routines for files in working directory#262
Conversation
haneug
left a comment
haneug
left a comment
There was a problem hiding this comment.
Nice PR. Please see my comments. It would also be good to add some unit tests to see which files get deleted.
| basename = self.basename | ||
| return self.working_dir / (basename + suffix) | ||
|
|
||
| def _delete_files(self, basename: str | None, *, suffixes: Sequence[str] | None = None) -> None: |
There was a problem hiding this comment.
- Any particular reason that
basenamehas no default?
I would assume having one makes the API a bit smoother. - I'm note huge fan to allow string parameters to also be
None, because an empty string typically acts as thenullvalue. Any particular reason to haveNonehere? - I would use an empty tuple as default for
Sequence-like parameters.
There was a problem hiding this comment.
I think having a None value makes it clear that the option of providing a basename other than self.basename is optional , and if its not given then OPI will default to self.basename. I also dont really think the API is changed much . I would like to maintain as much clarity as possible when it comes to deleting files which is why i added the None option.
I would however add a default of None
| if not basename: | ||
| raise ValueError("No basename specified") | ||
| files: Iterable[Path] | ||
| if suffixes is None: |
There was a problem hiding this comment.
What if the sequence is empty? Which behavior is expected then?
There was a problem hiding this comment.
If the sequences is empty the function would essentially be a no-op
3 participants
Closes Issues
Closes #258
Description
Release Notes
Added