Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove generated C files from the repository #31

Merged
merged 1 commit into from
Jul 21, 2024

Conversation

kalekundert
Copy link
Contributor

Currently, the repository contains 4 Cython-generated C source files. I think there are a few good reasons to remove these files:

  • This thread gives a variety of opinions on whether generated files should be committed or not. The consensus is that there are some niche reasons to do this (e.g. the hand-written code is tightly coupled to the generated code, or the code generator is unreliable), but generally you shouldn't. I don't think any of the niche reasons apply here.
  • Generated files can get out-of-date, and interfere with installation in confusing ways (as described in Fix the install process for pip>=23.1, python>=3.11, and/or Cython>=3.0 #26).
  • Any commit made with a different version of Cython will have really big diffs. This obfuscates what actually changes in each commit, and it can easily lead to merge conflicts as well.

I can think of one possible reason to not remove these files, which is that they make the software more reproducible by ensuring that everyone gets the same generated source. But I don't think this is a very compelling reason:

  • Without frequent updates (e.g. 2-3x per year), the generated source files get out-of-date and lead to the installation issues mentioned above. Frankly, this repository is just not updated frequently enough for this to work.
  • Distributing the generated source files doesn't require checking them into the repository. It just requires including those files in the archive uploaded to PyPI.

I expect that merging either this PR or #30 will cause merge conflicts for the other. If/when this happens, I'll fix the conflicts.

@tscohen tscohen merged commit cf2b3ec into AMLab-Amsterdam:master Jul 21, 2024
@tscohen
Copy link
Collaborator

tscohen commented Jul 21, 2024

I agree, it makes more sense to remove these. Not sure why I chose to include them.... 7 years ago :)

Thanks for your contribution!

@kalekundert kalekundert mentioned this pull request Jul 26, 2024
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.

2 participants