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

feat!: update vrs/common models #417

Merged
merged 13 commits into from
Jul 10, 2024
Merged

feat!: update vrs/common models #417

merged 13 commits into from
Jul 10, 2024

Conversation

korikuzma
Copy link
Contributor

@korikuzma korikuzma commented Jul 5, 2024

close #409 , #116

  • updates vrs/gks-common submodules
    • Pydantic models updated to reflect these changes
  • Removed use_enum_values from Pydantic config. We didn't use this in all models, so not sure if there was a purpose for it.
  • Removed duplicate vrs test

Could definitely get some other eyes on this to make sure I didn't miss anything

close #415
* only support 3 most recent python versions (3.10, 3.11, 3.12)
* change development python version from 3.10 to 3.12
* re-run cassettes
* ordered:false -> serializer should always sort the array before computing the digest so that regardless of the order, the digest always comes out the same
* ordered: true -> do not sort the items before digesting, so digests can be different based on ordering of array
@korikuzma korikuzma added enhancement New feature or request priority:high High priority labels Jul 5, 2024
@korikuzma korikuzma self-assigned this Jul 5, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering if we should split these out into core-im vs domain entity modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If so, might do this in a separate PR

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 deleted test_vrs.py and renamed test_vrs2.py to test_vrs.py. These were essentially the same, but test_vrs2.py had more tests.

@korikuzma korikuzma linked an issue Jul 5, 2024 that may be closed by this pull request
@korikuzma
Copy link
Contributor Author

Going to do one more look over in the morning. Then I think I'm ready to mark ready for review.

@korikuzma korikuzma changed the title feat!: update vrs/core models feat!: update vrs/common models Jul 9, 2024
@korikuzma korikuzma marked this pull request as ready for review July 9, 2024 13:26
@korikuzma korikuzma requested review from a team as code owners July 9, 2024 13:26
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.

looks good to me*

*someone who is not particularly involved in any of the changes to these models

@korikuzma korikuzma merged commit 506940e into main Jul 10, 2024
6 checks passed
@korikuzma korikuzma deleted the issue-409 branch July 10, 2024 14:30
korikuzma added a commit that referenced this pull request Jul 18, 2024
close #433

* Add back `use_enum_values` config . I removed in #417 because of a
copy/paste I had done in the Adjacency class which left me confused.
* `type` field should be a literal string, not enum
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.

Implement CisPhasedBlock Update ga4gh_identify to take the ordered property into consideration
2 participants