-
Notifications
You must be signed in to change notification settings - Fork 66
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(save): Save a dataset with multiple file arguments #718
Conversation
Fixes #690. |
3995794
to
9fe54bc
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.
Couple quick nits, this is looking really good!
@@ -57,7 +57,7 @@ commit message and title to the save.`, | |||
}, | |||
} | |||
|
|||
cmd.Flags().StringVarP(&o.FilePath, "file", "f", "", "dataset data file (yaml or json)") | |||
cmd.Flags().StringSliceVarP(&o.FilePaths, "file", "f", nil, "dataset or component file (yaml or json)") |
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.
it's currently valid to also supply .html
and .star
files here directly. Relevant code that handles those extensions is still lib.ReadDatasetFile
:
Lines 135 to 148 in b813d05
case ".star": | |
// starlark files are assumed to be a transform script with no additional | |
// tranform component details: | |
ds.Transform = &dataset.Transform{ScriptPath: path} | |
ds.Transform.SetScriptFile(qfs.NewMemfileReader("transform.star", f)) | |
return | |
case ".html": | |
// html files are assumped to be a viz script with no additional viz | |
// component details | |
ds.Viz = &dataset.Viz{ScriptPath: path} | |
ds.Viz.SetScriptFile(qfs.NewMemfileReader("viz.html", f)) | |
return | |
Any chance we can get cases that confirm both of those examples work? This is asking you to test code that I've written (😞), but we should confirm this behavior survives through this PR. The motivation is supplying no-configuration transform & viz components:
qri save --file transform.star me/dataset
qri save --file template.html me/dataset
I use both of these rather often, and find they make for easier tutorial writing.
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.
Added, along with an all-in-one combination of meta + transform + viz.
lib/file.go
Outdated
f *os.File | ||
data []byte | ||
) | ||
// ReadDatasetFiles decodes a dataset document into a Dataset |
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.
Outdated comment. It'd be great if this function comment broke down some of the nuances of reading multiple filepaths into a dataset, keeping it as high-level as possible. Something like this (adjusting for accuracy):
// ReadDatasetFiles combines resources at zero or more filepaths, deserializing them into a
// *dataset.Dataset. Files can separately specify different components of a dataset
// but two provided components cannot overlap (modify the same component)
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.
Done.
lib/file.go
Outdated
data []byte | ||
) | ||
// ReadDatasetFiles decodes a dataset document into a Dataset | ||
func ReadDatasetFiles(pathList []string) (*dataset.Dataset, error) { |
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'd prefer
func ReadDatasetFiles(pathList ... string) (*dataset.Dataset, error)
To me ReadDatasetFiles would be very useful when using qri-as-a-library by hiding the []string
from the caller:
// variadic arguments allows calls for easier single-file loading:
ds, err := lib.ReadDatasetFiles("./path.json")
// and chaining that doesn't require []string allocation:
ds, err := lib.ReadDatasetFiles("./structure.json", "./meta.yaml")
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.
Good idea, this is much better.
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.
DOPE
No description provided.