Only include changed files in PR preview #4405
Conversation
This reverts commit 3de94d2.
…H/preview-changed-files
| if "api" in contents: | ||
| ignores.append("api") |
There was a problem hiding this comment.
We need to no matter what ignore /api/, specifically with the main.yml workflow. Also, I would check for /api/ and not api because we have legit guides like primitives-rest-api
There was a problem hiding this comment.
This does do that: contents is a list of directory entries (e.g. ["api", "index.mdx", ... ]), so this has the effect of ignoring all paths matching /api/ and would not match primitives-rest-api. I think the confusion is because the interface of copytree's ignore is a bit unusual, I'll try to clarify in the comments.
| lines = Path(".github/outputs/changed-content-files.txt").read_text().split("\n") | ||
| changed_content_files = set(line for line in lines if line != "") | ||
| except FileNotFoundError: | ||
| changed_content_files: set[str] = set() |
There was a problem hiding this comment.
Maybe log (with logger)?
Eric-Arellano
left a comment
Eric-Arellano
left a comment
There was a problem hiding this comment.
Thanks! This overall looks great
| # Finally, continue traversing if the directory contains one of our | ||
| # changed files. |
There was a problem hiding this comment.
This comment makes me think that we will include all of the files in a directory if at least one is changed, i.e. sibling files. I don't think that's what the code is doing though. Maybe this?
| # Finally, continue traversing if the directory contains one of our | |
| # changed files. | |
| # Finally, include files that were directly changed. |
|
|
||
| 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.
2 participants
Closes #4366