Skip to content

Conversation

@dbaileychess
Copy link
Collaborator

Removes the bulk of the generated C++ files in our tests that are checked in, preferring them to be built on demand.

@aardappel
Copy link
Collaborator

Why?

How will we see what a PR changes without these files?

How will someone that refactors code quickly see if their changes still generate the same code as before? Etc.

The job of the software in this repo is code generation, thus generated files being in the repo makes a lot of sense to me.

@dbaileychess dbaileychess force-pushed the push-qpplqxuynrtv branch 2 times, most recently from 99bb659 to dbdd016 Compare December 22, 2025 03:48
@dbaileychess
Copy link
Collaborator Author

These are files tests depend on. If people don't remember to regenerate the files, there is a then a lack of test coverage.

I agree that we should have checked in code, and that is what the goldens/ directory is for. But we shouldn't use those dependencies.

@dbaileychess dbaileychess force-pushed the push-qpplqxuynrtv branch 10 times, most recently from b670c6e to aef6ac2 Compare December 22, 2025 05:33
@aardappel
Copy link
Collaborator

People forgetting to check in modified code is a much smaller problem than the issues I indicated. With the codegen and checking scripts this is not something that happens often.

Like I said, without all code affected being clear in a diff both development and review will be severely impacted. And we don't have as much coverage with what are currently designated as goldens, we need all generated files present.

Please don't merge this without some careful consideration of the consequences.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants