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: implement python query engine wrapper #229

Conversation

JabobKrauskopf
Copy link
Member

@JabobKrauskopf JabobKrauskopf commented Oct 11, 2024

This PR concludes the first phase of the query engine refactoring.

All tests should succeed again. Linting and Formatting should also not fail.

There was some functionality removed from the querying.pyi. This is tracked ind the epic issue #165 under Small usability improvemets.
There are no docstrings and no tests as of now. These will be added in the next phase. This is also tracked in #165

@JabobKrauskopf JabobKrauskopf force-pushed the 228-rewrite-python-query-engine-wrappers branch from 1699b17 to ec61375 Compare October 11, 2024 14:24
@JabobKrauskopf JabobKrauskopf force-pushed the 228-rewrite-python-query-engine-wrappers branch from ec61375 to 497b70f Compare October 11, 2024 14:30
@JabobKrauskopf JabobKrauskopf marked this pull request as ready for review October 11, 2024 14:31
@JabobKrauskopf JabobKrauskopf requested review from a team as code owners October 11, 2024 14:31
@JabobKrauskopf JabobKrauskopf linked an issue Oct 11, 2024 that may be closed by this pull request
Copy link
Contributor

@MarIniOnz MarIniOnz left a comment

Choose a reason for hiding this comment

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

We go to war with your PRs!

DALLE2024-10-1116 59 07-Acutesealstandinguprightsmilingandgivingathumbsupwhilewearingamilitaryoutfit Thesealisdressedinadetailedmilitaryuniformwith-ezgif com-webp-to-jpg-converter

medmodels/medrecord/querying.py Show resolved Hide resolved
medmodels/medrecord/querying.py Show resolved Hide resolved
medmodels/medrecord/querying.py Show resolved Hide resolved
@JabobKrauskopf JabobKrauskopf merged commit 76c234e into epic/165-query-engine-refactor-1 Oct 11, 2024
6 checks passed
@JabobKrauskopf JabobKrauskopf deleted the 228-rewrite-python-query-engine-wrappers branch October 11, 2024 16:09
Copy link
Contributor

@LauraBoenchenLB LauraBoenchenLB left a comment

Choose a reason for hiding this comment

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

Soooo excited for this PR and to start playing around with the queries ⭐ a few questions or remarks about naming, but it's not stopping the merge

@@ -50,7 +50,7 @@ def __repr__(self) -> str: ...
def __eq__(self, value: object) -> bool: ...

@staticmethod
def _from_pydatatype(datatype: PyDataType) -> DataType:
def _from_py_data_type(datatype: PyDataType) -> DataType:
Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly prefer the spelling before

medmodels/medrecord/querying.py Show resolved Hide resolved
def floor(self) -> None:
self._single_value_operand.floor()

def absolute(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

abs would fit in better with the other functions

@@ -422,7 +422,7 @@ def __init__(
)

@classmethod
def _from_pyschema(cls, schema: PySchema) -> Schema:
def _from_py_schema(cls, schema: PySchema) -> Schema:
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, I kinda see Pyschema as one word, but this opinion is not super strong

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.

Rewrite python query engine wrappers
3 participants