Skip to content

don't eagerly read file contents#751

Open
crockeo wants to merge 1 commit intocodegen-sh:developfrom
crockeo:ch/dont-eagerly-read-file-content
Open

don't eagerly read file contents#751
crockeo wants to merge 1 commit intocodegen-sh:developfrom
crockeo:ch/dont-eagerly-read-file-content

Conversation

@crockeo
Copy link
Copy Markdown

@crockeo crockeo commented Mar 5, 2025

Motivation

Many usages of iter_files don't explicitly include skip_content, but they don't actually care about the contents of the files. This can slow down codegen when running against a large repository with many large files.

Content

This PR just adds skip_content to all of the iter_files references I could find which don't care about the file content.

Note that this...actually is all of the references. The more "right" way to do this may be to change iter_files to never return the contents of files, but I didn't want to make that kind of change w/o talking to the maintainers first.

Testing

Best-effort testing locally. Would like to chat w/ y'all to figure out how to effectively test a change like this.

Please check the following before marking your PR as ready for review

  • I have added tests for my changes
  • I have updated the documentation or added new documentation as needed

@crockeo crockeo requested review from a team and codegen-team as code owners March 5, 2025 02:24
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Copy Markdown
Contributor

@EdwardJXLi EdwardJXLi left a comment

Choose a reason for hiding this comment

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

Hey there! Overall LGTM. Do you mind merging in from latest develop again? There's a couple changes that would fix the failing tests.

After that and the CLA check, I can then approve the pr 👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants