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: Initial support for apiv2 #1085

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

feat: Initial support for apiv2 #1085

wants to merge 18 commits into from

Conversation

jgadling
Copy link
Contributor

This is a checkpoint: Basic relationships and filters work, but I haven't had a chance to test all of the filter operators and update test infra yet.

Copy link
Contributor

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

Generally looks good, and I'm happy that the codegen module didn't need to be changed much.

After setting up an v2 backend, I was able to run some quick manual tests, so agreed this seems to be kinda working. Lots of the list relationships give empty lists, but that could just be due to the local test infra fakes.

With the new version of the backend, can we specify comments/descriptions on the item/list relationships? That was not possible with hasura, so I just had to autogenerate those descriptions with sometimes awkward results (e.g. The alignment this per section alignment parameters is a part of).

"tiltseries": "TiltSeries",
"depositions": "Deposition",
"deposition_authors": "DepositionAuthor",
"Alignment": "Alignment",
Copy link
Contributor

Choose a reason for hiding this comment

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

If these are always expected to be identical now, maybe this should just be a tuple of gql/model names instead of a mapping?

Comment on lines 258 to 260
of_type = get_named_type(
get_named_type(field_type.fields["edges"].type).fields["node"].type
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I find this chained call pretty hard to read. Consider unwrapping it with something like.

Suggested change
of_type = get_named_type(
get_named_type(field_type.fields["edges"].type).fields["node"].type
)
edges_type = get_named_type(field_type.fields["edges"].type)
of_type = get_named_type(edges_type.fields["node"].type)

@jgadling jgadling changed the title DRAFT: Initial support for apiv2 feat: Initial support for apiv2 Oct 17, 2024
@@ -135,7 +138,7 @@ def get_related_class(self):
return self.__current_query.get_related_class()

def to_gql(self):
return self.__name
return strcase.to_lower_camel(self.__name)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to cause an exception in strcase, as self.__name is a list, not a string.

to reproduce:

import cryoet_data_portal as cdp

client = cdp.Client()
point_annos = cdp.AnnotationFile.find(
    client,
    [
        cdp.AnnotationFile.tomogram_voxel_spacing.run_id == 1000,
        cdp.AnnotationFile.annotation_shape.shape_type._in(["Point", "OrientedPoint"]),  # noqa
    ],
)

TypeError: expected string or bytes-like object

Copy link
Contributor

Choose a reason for hiding this comment

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

Fix seems to be as easy as

return [strcase.to_lower_camel(n) for n in self.__name]

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.

3 participants