Skip to content

Comments

feat!: update models to vrs 2.0.0 community review ballot#471

Merged
korikuzma merged 12 commits intomainfrom
issue-469
Dec 20, 2024
Merged

feat!: update models to vrs 2.0.0 community review ballot#471
korikuzma merged 12 commits intomainfrom
issue-469

Conversation

@korikuzma
Copy link
Contributor

@korikuzma korikuzma commented Dec 18, 2024

close #469

  • Update modules to vrs 2.0.0-ballot.2024-11.3 tag
  • Rename class _Ga4ghIdentifiableObject to Ga4ghIdentifiableObject
  • Removes entity_models and domain_models from ga4gh.core. Now, all gks-core models are in ga4gh.core.models.
    • gks-core includes date and datetime as primitives, however the builtin datetime module already provides these. Initially, I kept them as Pydantic Root Models but I think we should just leverage the datetime module instead.

@korikuzma korikuzma added enhancement New feature or request priority:high High priority labels Dec 18, 2024
@korikuzma korikuzma self-assigned this Dec 18, 2024
@korikuzma korikuzma requested review from a team as code owners December 18, 2024 20:56
@korikuzma
Copy link
Contributor Author

VRS 2.0.0-ballot.2024-11.3 will be made later tonight. Will update submodules afterwards

Copy link
Contributor

@jsstevenson jsstevenson left a 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

path = submodules/vrs
url = https://github.com/ga4gh/vrs.git
branch = 2.x
branch = 2.0.0-ballot.2024-11
Copy link
Contributor

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)

Copy link
Contributor Author

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

Copy link
Contributor

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.

@jsstevenson jsstevenson self-requested a review December 20, 2024 16:25
Copy link
Contributor

@jsstevenson jsstevenson left a 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

@korikuzma korikuzma merged commit 01e3038 into main Dec 20, 2024
8 checks passed
@korikuzma korikuzma deleted the issue-469 branch December 20, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority:high High priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update models to VRS 2.0.0 Community Review Ballot

2 participants