Skip to content

Conversation

@jorisdral
Copy link
Collaborator

If the snapshot versioning scheme is to your liking, then we should probably also make it part of the PR checklist to update version numbers in case there are changes to the snapshot format.

@jorisdral jorisdral self-assigned this Oct 28, 2024
@jorisdral jorisdral changed the title Jdral/snapshot metadata Define snapshot versioning and metadata Oct 28, 2024
@jorisdral jorisdral force-pushed the jdral/snapshot-metadata branch from 642a493 to 6e5b3ae Compare November 4, 2024 15:47
@jorisdral jorisdral force-pushed the jdral/snapshot-metadata branch from 6e5b3ae to a84ca60 Compare November 4, 2024 21:00
@jorisdral jorisdral added this pull request to the merge queue Nov 5, 2024
@jorisdral jorisdral removed this pull request from the merge queue due to a manual request Nov 5, 2024
@jorisdral jorisdral force-pushed the jdral/snapshot-metadata branch from e549686 to 9a9adc5 Compare November 5, 2024 10:41
@jorisdral jorisdral enabled auto-merge November 5, 2024 10:41
@jorisdral jorisdral added this pull request to the merge queue Nov 5, 2024
Merged via the queue into main with commit a0b1abc Nov 5, 2024
24 checks passed
@jorisdral jorisdral deleted the jdral/snapshot-metadata branch November 5, 2024 11:48
Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM

import System.FS.BlockIO.API (HasBlockIO)

-- | Custom text to include in a snapshot file
newtype SnapshotLabel = SnapshotLabel Text
Copy link
Collaborator

Choose a reason for hiding this comment

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

A newtype is good but do we need Text here? This is the only thing that pulls in the extra text package dep.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Never mind, adding the dep on cborg in #443 adds text anyway.

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.

4 participants