-
Notifications
You must be signed in to change notification settings - Fork 0
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
Added coverage, updated chart_data lamdba #103
Conversation
dogversioning
commented
Sep 15, 2023
- Added coverage config
- Updated chart data lambda as part of addressing major coverage gap
- cleanup on migration 001
6756633
to
92ee176
Compare
I looked into a couple coverage PR actions but it's a decent amount of work to set up, so i'm leaving it for now in favor of using codecov once this is a public repo. |
Also - i'm not at 100% coverage right now, but i did review each gap - it's mostly error catching. I'll pick it up piecemeal as I go along. |
if highest_ver is None: | ||
highest_ver = ver_str | ||
else: | ||
if int(highest_ver) < int(ver_str): |
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.
nit: does this reduce boiler plate code, or is it too "clever"?
versions = []
if ...:
...
versions.append(int(ver_str))
return max(versions)
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.
i want the string, not the int - i could redo zero padding? i think that's about equally a pain.
|
||
|
||
def _get_table_cols(table_name: str) -> list: | ||
def _get_table_cols(table_name: str, version: str = None) -> list: |
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.
I don't see any of your code passing this new param in. Future plans?
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.
yeah, there's no need at the moment, but there is a distant future where the dashboard may want to request a specific version, rather than the latest.