Only include changed files in PR preview #4405
Conversation
This reverts commit 3de94d2.
…H/preview-changed-files
Eric-Arellano
left a comment
Eric-Arellano
left a comment
There was a problem hiding this comment.
Thanks! This overall looks great
|
|
||
| def _copy_local_content(root_dir: Path, changed_files: set[str]) -> None: | ||
|
|
||
| def ignore_contents(dir: str, contents: list[str]) -> list[str]: |
There was a problem hiding this comment.
Often I'm a fan of inlined functions, but here I think it makes the code more confusing than necessary. You aren't using anything from the outer closure, and the helper function is quite verbose. I recommend moving it to a standalone function.
There was a problem hiding this comment.
It does actually use the changed_files variable from the outer closure. Is there something else I can do to make it clearer?
There was a problem hiding this comment.
Ah. You could fix that though by adding a new changed_files arg to the function`. I'd still recommend splitting it out because both functions are now quite big
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Eric-Arellano
left a comment
Eric-Arellano
left a comment
There was a problem hiding this comment.
This needs to use the new approach for changed files
This reverts commit 489d238. The function closure includes `changed_files`
This reverts commit 63a4381.
2 participants
Closes #4366