search

Shopify / protoboeuf

Experimenting with a protobuf implementation
MIT License
37 stars 5 forks source link

Should we gitignore generated well_known_types? #153

Open rwstauner opened 2 months ago

rwstauner commented 2 months ago

In #149 I forgot to include the regenerated well_known_types, which led to a follow up PR #151. I proposed adding a check to CI to avoid it happening again #152.

However Ufuk asked

I wonder if we should not be committing these files to the repo, but making their generation a part of building the gem, in the first place?

One negative is losing an easy way to view the diff (of the result of codegen changes). One positive is that we wouldn't need the CI check because rake would actually build them after cloning.

If we add them to .gitignore but leave everything else the same, I think the experience would be the same even if you are doing development using a local clone as a dependency (as opposed to using a published gem version).

What other arguments are there for or against keeping them in the repo?

tenderworks commented 1 month ago

I wonder if we should not be committing these files to the repo, but making their generation a part of building the gem, in the first place?

I wonder this too. The main reason I can see to have them checked in is so that you can clone the repo and run an individual test like ruby -I lib:test test/blah.rb. Ruby wouldn't know to generate those files, people would have to run rake first and that might cause confusion during development.

One negative is losing an easy way to view the diff (of the result of codegen changes).

I like this too. It's nice to see how codegen changes.

tbh I'm not sure what we should do. My opinion isn't particularly strong. Theoretically, once code gen is stabilized for the most part, we won't be updating these files frequently and conflicts will be rare. OTOH, we're getting conflicts now and it's annoying. 🤷‍♀️

rwstauner commented 1 month ago

I agree with wanting to run tests without rake but that also opens the door for failing to regenerate when a change is made.

I do like seeing the diffs, though they are super large now.

You may be right, as changes stabilize maybe it won't be such a big deal 🤷