-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat!: update models to vrs 2.0.0 community review ballot #471
Conversation
VRS 2.0.0-ballot.2024-11.3 will be made later tonight. Will update submodules afterwards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 small requested changes (typos) and a few recommendations
@@ -1,4 +1,4 @@ | |||
[submodule "submodules/vrs"] | |||
path = submodules/vrs | |||
url = https://github.com/ga4gh/vrs.git | |||
branch = 2.x | |||
branch = 2.0.0-ballot.2024-11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason why we're pinning a branch and not a tag? I'd think we would want to be pinning specific commits. Submodules always hurt my brain though so maybe I'm misunderstanding.
(ditto for VRS -> gks-core, outside scope of this PR though obviously)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have missed something when googling before but tags aren't supported in .gitmodules
so I opted for the branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can punt this to an issue but I think commit would be appropriate (and it's effectively the same as a tag name). Otherwise there's a risk that changes to the submodule'd branch run ahead of what's expected in the superproject.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 reasonable on all fronts. looks good to merge on my end
close #469
_Ga4ghIdentifiableObject
toGa4ghIdentifiableObject
entity_models
anddomain_models
fromga4gh.core
. Now, all gks-core models are inga4gh.core.models
.date
anddatetime
as primitives, however the builtindatetime
module already provides these. Initially, I kept them as Pydantic Root Models but I think we should just leverage thedatetime
module instead.