-
Notifications
You must be signed in to change notification settings - Fork 2
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
Move database ownership to System #41
Conversation
daniel-thom
commented
Aug 22, 2024
- Move control over the database to the System instead of TimeSeriesMetadataStore. This will allow us to use the database for supplemental attributes.
- Use SQL query params for user variables.
|
||
|
||
class System: | ||
"""Implements behavior for systems""" | ||
|
||
DB_FILENAME = "time_series_metadata.db" |
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.
We should probably change the name of this. At serialization time, we store this file in a time series directory. So, it could be mildly confusing when supplemental attributes are added. We would have to add support for deserializing old systems. I'm voting that we delay this for now.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
==========================================
- Coverage 95.21% 95.07% -0.15%
==========================================
Files 31 32 +1
Lines 2529 2556 +27
==========================================
+ Hits 2408 2430 +22
- Misses 121 126 +5 ☔ View full report in Codecov by Sentry. |
f765026
to
fa2bb13
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.
Looks good to me!
Move database ownership to System