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

[save-] before saving, load unloaded sheets #2341

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

midichef
Copy link
Contributor

@midichef midichef commented Mar 5, 2024

repro:

vd sample_data/sample.tsv sample_data/errors.csv sample_data/countries
save-all
unloaded.vds

then
vd unloaded.vds will show that two of the sheets have no rows.

Also, saving a sheet with 0 rows results in a .vds file that's not readable. I'll file that as a separate issue.

@saulpw
Copy link
Owner

saulpw commented Mar 8, 2024

Should we fail outright, or call ensureLoaded() on them all and sync for them to finish?

@midichef
Copy link
Contributor Author

Good point, loading is definitely better. I've changed it to call ensureLoaded() instead of fail.

@midichef midichef changed the title [save-] refuse to save unloaded sheets as .vds [save-] before saving, load unloaded sheets Mar 12, 2024
@@ -109,6 +109,8 @@ def saveSheets(vd, givenpath, *vsheets, confirm_overwrite=True):
if not vsheets: # blank tuple
vd.warning('no sheets to save')
return
unloaded = [ vs for vs in vsheets if vs.rows is UNLOADED ]
vd.sync(*vd.ensureLoaded(unloaded))
Copy link
Owner

Choose a reason for hiding this comment

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

Closer! But ensureLoaded is on BaseSheet, not the vd object. And it also checks for UNLOADED itself. So maybe we should make vd.sync ignore None values, and just call vd.sync(*[vs.ensureLoaded() for vs in vsheets]).

@anjakefala
Copy link
Collaborator

This is so cool!

@anjakefala anjakefala merged commit c8162d9 into saulpw:develop Mar 17, 2024
13 checks passed
@midichef midichef deleted the vds_unloaded branch March 23, 2024 01:53
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.

None yet

3 participants