Warn users to save their metadata file after auto-detecting/updating it #1786
Conversation
R-Palazzo
force-pushed
the
issue-1762-save-metadata
branch
from
11f9b9b to
e2e031b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1786 +/- ##
==========================================
+ Coverage 97.25% 97.27% +0.02%
==========================================
Files 49 49
Lines 4624 4662 +38
==========================================
+ Hits 4497 4535 +38
Misses 127 127 ☔ View full report in Codecov by Sentry. |
sdv/metadata/single_table.py
Outdated
| @@ -1069,6 +1082,7 @@ def load_from_json(cls, filepath): | |||
| 'class and version.' | |||
| ) | |||
|
|
|||
| cls._updated = False | |||
There was a problem hiding this comment.
why not make the default false?
There was a problem hiding this comment.
Yes, done in 18ee971
sdv/multi_table/base.py
Outdated
| self.metadata._check_updated_flag() | ||
| if self.metadata._updated: |
There was a problem hiding this comment.
Not sure if you need to store the _updated flag if you can just directly call _check_metadata_updated
There was a problem hiding this comment.
Yes, I did it to be consistent between single and multi-table metadata. I removed it in d20f32a
sdv/metadata/multi_table.py
Outdated
| @@ -29,6 +29,20 @@ class MultiTableMetadata: | |||
| def __init__(self): | |||
| self.tables = {} | |||
| self.relationships = [] | |||
| self._multi_table_updated = None | |||
There was a problem hiding this comment.
Can we have the default be False here for consistency? We could also rename it to just _updated, but I'm not sure if that would be more or less confusing.
There was a problem hiding this comment.
Yes, done in 6b0c5cf
sdv/metadata/multi_table.py
Outdated
| table._detect_columns(data) | ||
| table._updated = True |
There was a problem hiding this comment.
Is there a reason we call table._detect_columns here instead of just table.detect_from_dataframe? We wouldn't need to set the table's updated flag here if we use the public method.
There was a problem hiding this comment.
Before we seem to be using table.detect_from_dataframe from this: d1ae6de
Maybe @amontanez24 remembers a reason for it.
There was a problem hiding this comment.
Oh yeah, looks like we use it to avoid double logging the detected table. In that case, we could move the single table _updated flag switch to _detect_columns instead of detect_from_dataframe
There was a problem hiding this comment.
Yes, done in 6b0c5cf
CU-86az6hhgm
Resolve #1762