From 0cd35d90ea4dd0fb4cc22580aedcb94fc49f6f48 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 21 Nov 2024 00:43:16 +0100 Subject: [PATCH] address comments --- bundle/config/mutator/translate_paths.go | 38 +++++--- bundle/config/mutator/translate_paths_test.go | 87 +++++++++++++++---- 2 files changed, 95 insertions(+), 30 deletions(-) diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index 58c088b8..5733d0a6 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -126,19 +126,35 @@ func (t *translateContext) rewritePath( func (t *translateContext) translateNotebookPath(literal, localFullPath, localRelPath, remotePath string) (string, error) { nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, filepath.ToSlash(localRelPath)) if errors.Is(err, fs.ErrNotExist) { - if filepath.Ext(localFullPath) == notebook.ExtensionNone { - return "", fmt.Errorf(`notebook %s not found. Local notebook references are expected -to contain one of the following file extensions: [%s]`, - literal, - strings.Join([]string{ - notebook.ExtensionPython, - notebook.ExtensionR, - notebook.ExtensionScala, - notebook.ExtensionSql, - notebook.ExtensionJupyter}, ", ")) + if filepath.Ext(localFullPath) != notebook.ExtensionNone { + return "", fmt.Errorf("notebook %s not found", literal) } - return "", fmt.Errorf("notebook %s not found", literal) + extensions := []string{ + notebook.ExtensionPython, + notebook.ExtensionR, + notebook.ExtensionScala, + notebook.ExtensionSql, + notebook.ExtensionJupyter, + } + + // Check whether a file with a notebook extension already exists. This + // way we can provide a more targeted error message. + for _, ext := range extensions { + literalWithExt := literal + ext + localRelPathWithExt := filepath.ToSlash(localRelPath + ext) + // TODO: inline the err call. + _, err := fs.Stat(t.b.SyncRoot, localRelPathWithExt) + if err == nil { + return "", fmt.Errorf(`notebook %s not found. Did you mean %s? +Local notebook references are expected to contain one of the following +file extensions: [%s]`, literal, literalWithExt, strings.Join(extensions, ", ")) + } + } + + // Return a generic error message if no matching possible q is found. + return "", fmt.Errorf(`notebook %s not found. Local notebook references are expected +to contain one of the following file extensions: [%s]`, literal, strings.Join(extensions, ", ")) } if err != nil { return "", fmt.Errorf("unable to determine if %s is a notebook: %w", localFullPath, err) diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index ab54de81..f5c1bf41 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -2,6 +2,7 @@ package mutator_test import ( "context" + "fmt" "os" "path/filepath" "runtime" @@ -509,35 +510,83 @@ func TestPipelineNotebookDoesNotExistError(t *testing.T) { } func TestPipelineNotebookDoesNotExistErrorWithoutExtension(t *testing.T) { - dir := t.TempDir() + for _, ext := range []string{ + ".py", + ".r", + ".scala", + ".sql", + ".ipynb", + "", + } { + t.Run("case_"+ext, func(t *testing.T) { + dir := t.TempDir() - b := &bundle.Bundle{ - SyncRootPath: dir, - SyncRoot: vfs.MustNew(dir), - Config: config.Root{ - Resources: config.Resources{ - Pipelines: map[string]*resources.Pipeline{ - "pipeline": { - PipelineSpec: &pipelines.PipelineSpec{ - Libraries: []pipelines.PipelineLibrary{ - { - Notebook: &pipelines.NotebookLibrary{ - Path: "./doesnt_exist", + if ext != "" { + touchEmptyFile(t, filepath.Join(dir, "foo"+ext)) + } + + b := &bundle.Bundle{ + SyncRootPath: dir, + SyncRoot: vfs.MustNew(dir), + Config: config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "pipeline": { + PipelineSpec: &pipelines.PipelineSpec{ + Libraries: []pipelines.PipelineLibrary{ + { + Notebook: &pipelines.NotebookLibrary{ + Path: "./foo", + }, + }, }, }, }, }, }, }, - }, - }, + } + + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "fake.yml")}}) + diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) + + if ext == "" { + assert.EqualError(t, diags.Error(), `notebook ./foo not found. Local notebook references are expected +to contain one of the following file extensions: [.py, .r, .scala, .sql, .ipynb]`) + } else { + assert.EqualError(t, diags.Error(), fmt.Sprintf(`notebook ./foo not found. Did you mean ./foo%s? +Local notebook references are expected to contain one of the following +file extensions: [.py, .r, .scala, .sql, .ipynb]`, ext)) + } + }) } - bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "fake.yml")}}) + // touchEmptyFile(t, filepath.Join(dir, "doesnt_exist.py")) + + // b := &bundle.Bundle{ + // SyncRootPath: dir, + // SyncRoot: vfs.MustNew(dir), + // Config: config.Root{ + // Resources: config.Resources{ + // Pipelines: map[string]*resources.Pipeline{ + // "pipeline": { + // PipelineSpec: &pipelines.PipelineSpec{ + // Libraries: []pipelines.PipelineLibrary{ + // { + // Notebook: &pipelines.NotebookLibrary{ + // Path: "./doesnt_exist", + // }, + // }, + // }, + // }, + // }, + // }, + // }, + // }, + // } + + // bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "fake.yml")}}) - diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) - assert.EqualError(t, diags.Error(), `notebook ./doesnt_exist not found. Local notebook references are expected -to contain one of the following file extensions: [.py, .r, .scala, .sql, .ipynb]`) } func TestPipelineFileDoesNotExistError(t *testing.T) {