Skip to content

Conversation

@frankharkins
Copy link
Member

@frankharkins frankharkins commented Dec 12, 2025

Closes #4366

Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

frankharkins and others added 3 commits December 31, 2025 18:13
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to use the new approach for changed files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

PR preview only has changed files

2 participants