fix(status): Status displays parse errors, instead of bailing out

cmd/fsi_test.go

@@ -449,6 +449,74 @@ run ` + "`qri save`" + ` to commit this dataset
  }
 }
 
+// Test that status displays parse errors correctly
+func TestStatusParseError(t *testing.T) {
+ if err := confirmQriNotRunning(); err != nil {
+ t.Skip(err.Error())
+ }
+
+ r := NewTestRepoRoot(t, "qri_test_status_parse_error")
+ defer r.Delete()
+
+ ctx, done := context.WithCancel(context.Background())
+ defer done()
+
+ pwd, err := os.Getwd()
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ rootPath, err := ioutil.TempDir("", "")
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ // Save a dataset containing a body.json and meta component
+ cmdR := r.CreateCommandRunner(ctx)
+ err = executeCommand(cmdR, "qri save --body=testdata/movies/body_two.json --file=testdata/movies/meta_override.yaml me/bad_movies")
+ if err != nil {
+ t.Fatalf(err.Error())
+ }
+
+ // Change to a temporary directory.
+ os.Chdir(rootPath)
+ defer os.Chdir(pwd)
+
+ // Checkout the newly created dataset.
+ cmdR = r.CreateCommandRunner(ctx)
+ err = executeCommand(cmdR, "qri checkout me/bad_movies")
+ if err != nil {
+ t.Fatalf(err.Error())
+ }
+
+ workPath := filepath.Join(rootPath, "bad_movies")
+ os.Chdir(workPath)
+ defer os.Chdir(pwd)
+
+ // Modify the meta.json so that it fails to parse.
+ if err = ioutil.WriteFile("meta.json", []byte(`{"title": "hello}`), os.ModePerm); err != nil {
+ t.Fatalf(err.Error())
+ }
+
+ // Status, check that status shows the parse error.
+ cmdR = r.CreateCommandRunner(ctx)
+ err = executeCommand(cmdR, "qri status")
+ if err != nil {
+ t.Fatalf(err.Error())
+ }
+
+ output := r.GetOutput()
+ expect := `for dataset [test_peer/bad_movies]
+
+ parse_error: meta (source: meta.json)
+
+fix these problems before saving this dataset
+`
+ if diff := cmpTextLines(expect, output); diff != "" {
+ t.Errorf("qri status (-want +got):\n%s", diff)
+ }
+}
+
 func cmpTextLines(left, right string) string {
  lside := strings.Split(left, "\n")
  rside := strings.Split(right, "\n")

cmd/save.go

@@ -113,11 +113,15 @@ func (o *SaveOptions) Complete(f Factory, args []string) (err error) {
  return
  }
  if o.Refs.IsLinked() {
+ var problems map[string]string
  o.FSIMethods = lib.NewFSIMethods(f.Instance())
- o.fsids, _, err = fsi.ReadDir(o.Refs.Dir())
+ o.fsids, _, problems, err = fsi.ReadDir(o.Refs.Dir())
  if err != nil {
  return
  }
+ if problems != nil {
+ return fmt.Errorf("cannot save, dataset has errors")
+ }
  }
 
  // Make all paths absolute. Especially important if we are running

cmd/status.go

@@ -78,6 +78,10 @@ func (o *StatusOptions) Run() (err error) {
  case fsi.STAdd, fsi.STChange:
  printErr(o.Out, fmt.Errorf(" %s: %s (source: %s)", si.Type, si.Component, filepath.Base(si.SourceFile)))
  clean = false
+ case fsi.STParseError:
+ printErr(o.Out, fmt.Errorf(" %s: %s (source: %s)", si.Type, si.Component, filepath.Base(si.SourceFile)))
+ clean = false
+ valid = false
  }
  // TODO(dlong): Validate each file / component, set `valid` to false if any problems exist
  }
@@ -86,6 +90,8 @@ func (o *StatusOptions) Run() (err error) {
  printSuccess(o.Out, "working directory clean")
  } else if valid {
  printSuccess(o.Out, "\nrun `qri save` to commit this dataset")
+ } else {
+ printErr(o.Out, fmt.Errorf("\nfix these problems before saving this dataset"))
  }
  return nil
 }

fsi/mapping.go

@@ -32,10 +32,11 @@ var (
 // a map of component names to the files they came from. Files can be specified
 // in either JSON or YAML format. It is an error to specify any component more
 // than once
-func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, err error) {
+func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, problems map[string]string, err error) {
  mapping = map[string]string{}
  ds = &dataset.Dataset{}
  schema := map[string]interface{}{}
+ problems = nil
 
  components := map[string]interface{}{
  componentNameDataset: ds,
@@ -75,7 +76,7 @@ func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, err er
  }
  if _, err = os.Stat(filepath.Join(dir, "body.json")); !os.IsNotExist(err) {
  if bodyFormat == "csv" {
- return ds, mapping, fmt.Errorf("body.csv and body.json both exist")
+ return ds, mapping, problems, fmt.Errorf("body.csv and body.json both exist")
  }
  bodyFormat = "json"
  }
@@ -84,7 +85,7 @@ func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, err er
  if bodyFormat != "" {
  bodyFilename = fmt.Sprintf("body.%s", bodyFormat)
  if err = addMapping(componentNameBody, bodyFilename); err != nil {
- return ds, mapping, err
+ return ds, mapping, problems, err
  }
  if ds.BodyPath == "" {
  ds.BodyPath = filepath.Join(dir, bodyFilename)
@@ -101,11 +102,14 @@ func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, err er
  if f, e := os.Open(path); e == nil {
  if cmpName != componentNameBody {
  if err = mkDec(f).Decode(cmp); err != nil {
- err = fmt.Errorf("reading %s: %s", filename, err)
- return ds, mapping, err
+ if problems == nil {
+ problems = make(map[string]string)
+ }
+ problems[cmpName] = filename
+ continue
  }
  if err = addMapping(cmpName, path); err != nil {
- return ds, mapping, err
+ return ds, mapping, problems, err
  }
  }
 
@@ -178,7 +182,7 @@ func ReadDir(dir string) (ds *dataset.Dataset, mapping map[string]string, err er
  err = ErrNoDatasetFiles
  }
 
- return ds, mapping, err
+ return ds, mapping, problems, err
 }
 
 type decoderFactory func(io.Reader) decoder

fsi/mapping_test.go

@@ -16,7 +16,7 @@ func TestReadDir(t *testing.T) {
 
  for _, c := range good {
  t.Run(fmt.Sprintf("good: %s", filepath.Base(c.path)), func(t *testing.T) {
- _, _, err := ReadDir(c.path)
+ _, _, _, err := ReadDir(c.path)
  if err != nil {
  t.Errorf("expected no error. got: %s", err)
  }
@@ -34,7 +34,7 @@ func TestReadDir(t *testing.T) {
 
  for _, c := range bad {
  t.Run(fmt.Sprintf("bad: %s", filepath.Base(c.path)), func(t *testing.T) {
- _, _, err := ReadDir(c.path)
+ _, _, _, err := ReadDir(c.path)
  t.Log(err)
  if err == nil {
  t.Errorf("expected error. got: %s", err)

fsi/status.go

@@ -24,6 +24,8 @@ var (
  STChange = "modified"
  // STRemoved is a removed component
  STRemoved = "removed"
+ // STParseError is a component that didn't parse
+ STParseError = "parse_error"
 )
 
 // StatusItem is a component that has status representation on the filesystem
@@ -127,7 +129,7 @@ func (fsi *FSI) Status(dir string) (changes []StatusItem, err error) {
  stored.Transform = nil
  stored.Peername = ""
 
- working, mapping, err := ReadDir(dir)
+ working, mapping, problems, err := ReadDir(dir)
  if err != nil {
  return nil, err
  }
@@ -143,6 +145,20 @@ func (fsi *FSI) Status(dir string) (changes []StatusItem, err error) {
  // when reporting deletes, ignore "bound" components that must/must-not
  // exist based on external conditions
  if cmpName != componentNameDataset && cmpName != componentNameStructure && cmpName != componentNameCommit && cmpName != componentNameViz {
+
+ if problems != nil {
+ // Problems is nil unless some components have errors.
+ if cmpFilename, ok := problems[cmpName]; ok {
+ change := StatusItem{
+ SourceFile: cmpFilename,
+ Component: cmpName,
+ Type: STParseError,
+ }
+ changes = append(changes, change)
+ continue
+ }
+ }
+
  cmp := dsComponent(stored, cmpName)
  // If the component was not in the previous version, it can't have been removed.
  if cmp == nil {