-
Notifications
You must be signed in to change notification settings - Fork 55
Catalog Endpoints Part 1 #926
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for neo4j-graph-data-science-client canceled.
|
41eba1a
to
4f96f34
Compare
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.
Nice
Mainly smaller suggestions
class GraphListResult(BaseModel): | ||
model_config = ConfigDict(alias_generator=to_camel) |
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.
class GraphListResult(BaseModel): | |
model_config = ConfigDict(alias_generator=to_camel) | |
class GraphListResult(BaseModel, alias_generator=to_camel): |
its allowed to be passed like this.
Easier to not confuse with the actual fields.
(see for reference -- https://github.com/neo4j/python-graph-visualization/blob/553f47130b514d7d93f5c3206d1eb9a32958b846/python-wrapper/src/neo4j_viz/node.py#L29)
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.
Uh that is nice. I was generally thinking if we might want to introduce our own base class for all those results, that one could already define all the necessary options. WDYT?
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.
makes sense to me 👍
class GraphFilterResult(BaseModel): | ||
model_config = ConfigDict(alias_generator=to_camel) |
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.
class GraphFilterResult(BaseModel): | |
model_config = ConfigDict(alias_generator=to_camel) | |
class GraphFilterResult(BaseModel, alias_generator=to_camel)): |
graphdatascience/procedure_surface/arrow/catalog_arrow_endpoints.py
Outdated
Show resolved
Hide resolved
graphdatascience/procedure_surface/arrow/catalog_arrow_endpoints.py
Outdated
Show resolved
Hide resolved
|
||
|
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.
no project?
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.
No, project is very implementation dependent, for Cypher+Plugin it will be native projection, Arrow+Session -> remote projection, Arrow + Plugin?? that is why I skipped that.
graphdatascience/tests/integrationV2/procedure_surface/arrow/conftest.py
Outdated
Show resolved
Hide resolved
graphdatascience/tests/integrationV2/procedure_surface/arrow/conftest.py
Outdated
Show resolved
Hide resolved
graphdatascience/tests/integrationV2/procedure_surface/arrow/conftest.py
Outdated
Show resolved
Hide resolved
graphdatascience/tests/integrationV2/procedure_surface/arrow/conftest.py
Outdated
Show resolved
Hide resolved
graphdatascience/tests/integrationV2/procedure_surface/arrow/test_catalog_arrow_endpoints.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Florentin Dörre <[email protected]>
Co-authored-by: Florentin Dörre <[email protected]>
Co-authored-by: Florentin Dörre <[email protected]>
66f3c1b
to
0646005
Compare
Co-authored-by: Florentin Dörre <[email protected]>
ref GDSA-75