address comments

This commit is contained in:
Shreyas Goenka 2024-11-21 00:43:16 +01:00
parent 5b77be186e
commit 0cd35d90ea
No known key found for this signature in database
GPG Key ID: 92A07DF49CCB0622
2 changed files with 95 additions and 30 deletions

View File

@ -126,19 +126,35 @@ func (t *translateContext) rewritePath(
func (t *translateContext) translateNotebookPath(literal, localFullPath, localRelPath, remotePath string) (string, error) { func (t *translateContext) translateNotebookPath(literal, localFullPath, localRelPath, remotePath string) (string, error) {
nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, filepath.ToSlash(localRelPath)) nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, filepath.ToSlash(localRelPath))
if errors.Is(err, fs.ErrNotExist) { if errors.Is(err, fs.ErrNotExist) {
if filepath.Ext(localFullPath) == notebook.ExtensionNone { if filepath.Ext(localFullPath) != notebook.ExtensionNone {
return "", fmt.Errorf(`notebook %s not found. Local notebook references are expected return "", fmt.Errorf("notebook %s not found", literal)
to contain one of the following file extensions: [%s]`, }
literal,
strings.Join([]string{ extensions := []string{
notebook.ExtensionPython, notebook.ExtensionPython,
notebook.ExtensionR, notebook.ExtensionR,
notebook.ExtensionScala, notebook.ExtensionScala,
notebook.ExtensionSql, notebook.ExtensionSql,
notebook.ExtensionJupyter}, ", ")) notebook.ExtensionJupyter,
} }
return "", fmt.Errorf("notebook %s not found", literal) // 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 { if err != nil {
return "", fmt.Errorf("unable to determine if %s is a notebook: %w", localFullPath, err) return "", fmt.Errorf("unable to determine if %s is a notebook: %w", localFullPath, err)

View File

@ -2,6 +2,7 @@ package mutator_test
import ( import (
"context" "context"
"fmt"
"os" "os"
"path/filepath" "path/filepath"
"runtime" "runtime"
@ -509,8 +510,21 @@ func TestPipelineNotebookDoesNotExistError(t *testing.T) {
} }
func TestPipelineNotebookDoesNotExistErrorWithoutExtension(t *testing.T) { func TestPipelineNotebookDoesNotExistErrorWithoutExtension(t *testing.T) {
for _, ext := range []string{
".py",
".r",
".scala",
".sql",
".ipynb",
"",
} {
t.Run("case_"+ext, func(t *testing.T) {
dir := t.TempDir() dir := t.TempDir()
if ext != "" {
touchEmptyFile(t, filepath.Join(dir, "foo"+ext))
}
b := &bundle.Bundle{ b := &bundle.Bundle{
SyncRootPath: dir, SyncRootPath: dir,
SyncRoot: vfs.MustNew(dir), SyncRoot: vfs.MustNew(dir),
@ -522,7 +536,7 @@ func TestPipelineNotebookDoesNotExistErrorWithoutExtension(t *testing.T) {
Libraries: []pipelines.PipelineLibrary{ Libraries: []pipelines.PipelineLibrary{
{ {
Notebook: &pipelines.NotebookLibrary{ Notebook: &pipelines.NotebookLibrary{
Path: "./doesnt_exist", Path: "./foo",
}, },
}, },
}, },
@ -534,10 +548,45 @@ func TestPipelineNotebookDoesNotExistErrorWithoutExtension(t *testing.T) {
} }
bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "fake.yml")}}) bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(dir, "fake.yml")}})
diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths())
assert.EqualError(t, diags.Error(), `notebook ./doesnt_exist not found. Local notebook references are expected
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]`) 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))
}
})
}
// 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")}})
} }
func TestPipelineFileDoesNotExistError(t *testing.T) { func TestPipelineFileDoesNotExistError(t *testing.T) {