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!: use abstract interface for DB #378

Merged
merged 35 commits into from
Dec 4, 2023
Merged

feat!: use abstract interface for DB #378

merged 35 commits into from
Dec 4, 2023

Conversation

jsstevenson
Copy link
Member

@jsstevenson jsstevenson commented Oct 31, 2023

close #343
progress on #254

This PR does similar work as the Postgres PRs for the other normalizers, but just updates the interface and related elements for DynamoDB (postgres will be a future PR, it's been giving me some troubles and I might need to rethink how it was implemented in the other normalizers).

src/therapy/query.py Outdated Show resolved Hide resolved
@jsstevenson jsstevenson marked this pull request as ready for review November 29, 2023 18:35
@jsstevenson jsstevenson added the priority:low Low priority label Nov 29, 2023
korikuzma
korikuzma previously approved these changes Dec 4, 2023
src/therapy/database/database.py Outdated Show resolved Hide resolved
def __init__(self, db_url: Optional[str] = None, **db_args) -> None:
"""Initialize Database class.

:param str db_url: URL endpoint for DynamoDB source
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:param str db_url: URL endpoint for DynamoDB source
:param db_url: URL endpoint for DynamoDB source

Comment on lines 236 to 241
:param str concept_id: concept ID for therapy record
:param bool case_sensitive: if true, performs exact lookup, which is more
efficient. Otherwise, performs filter operation, which doesn't require
correct casing.
:param bool merge: if true, look for merged record; look for identity record
otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:param str concept_id: concept ID for therapy record
:param bool case_sensitive: if true, performs exact lookup, which is more
efficient. Otherwise, performs filter operation, which doesn't require
correct casing.
:param bool merge: if true, look for merged record; look for identity record
otherwise.
:param concept_id: concept ID for therapy record
:param case_sensitive: if true, performs exact lookup, which is more
efficient. Otherwise, performs filter operation, which doesn't require
correct casing.
:param merge: if true, look for merged record; look for identity record
otherwise.

def add_record(self, record: Dict, src_name: SourceName) -> None:
"""Add new record to database.

:param Dict record: record to upload
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:param Dict record: record to upload
:param record: record to upload

Comment on lines 496 to 499
:param str term: referent term
:param str concept_id: concept ID to refer to
:param str ref_type: one of {'alias', 'label', 'xref',
'associated_with'}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:param str term: referent term
:param str concept_id: concept ID to refer to
:param str ref_type: one of {'alias', 'label', 'xref',
'associated_with'}
:param term: referent term
:param concept_id: concept ID to refer to
:param ref_type: one of {'alias', 'label', 'xref',
'associated_with'}

Co-authored-by: Kori Kuzma <korikuzma@gmail.com>
@jsstevenson jsstevenson merged commit 8e1f51a into main Dec 4, 2023
16 checks passed
@jsstevenson jsstevenson deleted the db-interface branch December 4, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:low Low priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Database naming updates
2 participants