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

Pydantic V2 migration #16

Merged
merged 11 commits into from
Nov 11, 2024
Merged

Pydantic V2 migration #16

merged 11 commits into from
Nov 11, 2024

Conversation

nvictus
Copy link
Member

@nvictus nvictus commented Nov 10, 2024

Resolves #15

@nvictus nvictus requested a review from manzt November 10, 2024 14:36
@nvictus
Copy link
Member Author

nvictus commented Nov 10, 2024

@manzt Running the validator using the 2020-12 spec, there seems to be just one issue:

schema schema.json is invalid
error: schema is invalid: data/$id must match pattern "^[^#]*#?$"

It doesn't like # fragments in $ids. How would you like to rename it?

@manzt
Copy link
Member

manzt commented Nov 11, 2024

HiGlass validates with ajv in the front end. Needing to change the test to accommodate the new schema version isn't ideal, but I guess we really aren't using this schema anywhere (since higlass still uses the manually edited schema.json and we never migrated).

Copy link
Member

@manzt manzt 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—left a couple of comments. There wasn’t much context in the issue or PR, so I just want to confirm: is this ready for final review, or are there more changes coming? Also, was this migration prompted by any specific issues with Pydantic v1, or is it just to stay up to date?

Obviously this introduces breaking changes for higlass-python, so any context would be helpful.

("$schema", "http://json-schema.org/draft-07/schema#"),
("$id", "https://higlass.io/#viewconf"),
("$schema", GenerateJsonSchema.schema_dialect),
("$id", "https://higlass.io/schemas/viewconf"),
Copy link
Member

Choose a reason for hiding this comment

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

The "$id" field in JSON Schema is used to define a unique identifier (URI) for the schema. I copied the previous one from the original schema, but apparently it's not a real link.

Is the URL you specified supposed to be a real link to the schema? Otherwise I say we remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mine was made up too.

@nvictus
Copy link
Member Author

nvictus commented Nov 11, 2024

Sorry for the lack of context! The migration was prompted by dependency conflicts between higlass-python and other packages in my environments since other packages have migrated to V2. It's ready for final review.

@manzt
Copy link
Member

manzt commented Nov 11, 2024

Sounds good, I've made some changes on a separate branch to align the JSON schema generation because it's quite a bit different with this PR.

I'll merge and open a separate PR.

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.

Pydantic 2 migration
2 participants