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

Don't add snapshot_kind: text to snapshots #690

Merged
merged 7 commits into from
Dec 13, 2024

Conversation

max-sixty
Copy link
Collaborator

This was extremely simple, sorry to leave it hanging.

Will do a self-review tomorrow to confirm I haven't missed anything

@nyurik
Copy link

nyurik commented Dec 10, 2024

horray, thanks!!

@max-sixty
Copy link
Collaborator Author

One thing I did miss — this will remove existing snapshot_kind: text. Ideally we'd keep those, or it's going to be a lot more churn.

(shows the cost of leaving it for a while — I remember thinking this at the time)

@max-sixty
Copy link
Collaborator Author

OK, I realize it's not tractable to have it retain snapshot_kind: text. So now making changes to any snapshots (or running --force-update-snapshots) that have snapshot_kind: text will remove that metadata item.

I personally disagree with merging this. From the attached issue explaining the original change:

I appreciate it's some aesthetic churn. But OTOH it only appears when updating snapshots. And for those who care about the cleanliness of diffs, doing a single run of --force-update-snapshots will update everything in a single commit. And in general it's important for insta to be able to evolve the snapshot format, albeit in the specific case we could have traded away consistency in metadata to reduce changes.

Merging this adds some additional churn (though admittedly more so since I took a couple weeks to do this).

But I weigh @mitsuhiko 's opinion highly, and appreciate some of the trade for being able to move quickly is occasionally reverting things. So I'll merge now, and release in a few days unless there are any second thoughts.

@max-sixty max-sixty merged commit 0283774 into mitsuhiko:master Dec 13, 2024
15 checks passed
@max-sixty max-sixty deleted the snapshot_kind branch December 13, 2024 18:07
@nyurik
Copy link

nyurik commented Dec 13, 2024

thx for merging!!! Can't wait to switch to the new version :)

CommanderStorm added a commit to maplibre/martin that referenced this pull request Dec 13, 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