From 19a20512e9870859113f1c28755d39c7695e7fd5 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 5 Jun 2023 13:49:33 +0200 Subject: [PATCH 1/7] comments --- cmd/fs/ls.go | 36 +++++++++++++++++++++++++++++------- cmd/fs/ls_output.go | 27 --------------------------- cmd/root/io.go | 12 +++++------- libs/cmdio/io.go | 28 ++++++++++++---------------- 4 files changed, 46 insertions(+), 57 deletions(-) delete mode 100644 cmd/fs/ls_output.go diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 76b51ce7e..1df8dfcf4 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -1,7 +1,9 @@ package fs import ( + "io/fs" "sort" + "time" "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/libs/cmdio" @@ -9,6 +11,27 @@ import ( "github.com/spf13/cobra" ) +type jsonDirEntry struct { + Name string `json:"name"` + IsDir bool `json:"is_directory"` + Size int64 `json:"size"` + ModTime time.Time `json:"last_modified"` +} + +func toJsonDirEntry(f fs.DirEntry) (*jsonDirEntry, error) { + info, err := f.Info() + if err != nil { + return nil, err + } + + return &jsonDirEntry{ + Name: f.Name(), + IsDir: f.IsDir(), + Size: info.Size(), + ModTime: info.ModTime(), + }, nil +} + // lsCmd represents the ls command var lsCmd = &cobra.Command{ Use: "ls ", @@ -17,10 +40,6 @@ var lsCmd = &cobra.Command{ Args: cobra.ExactArgs(1), PreRunE: root.MustWorkspaceClient, Annotations: map[string]string{ - "template_long": cmdio.Heredoc(` - {{range .}}{{if .IsDir}}DIRECTORY {{else}}FILE {{end}}{{.Size}} {{.ModTime|pretty_date}} {{.Name}} - {{end}} - `), "template": cmdio.Heredoc(` {{range .}}{{.Name}} {{end}} @@ -46,9 +65,9 @@ var lsCmd = &cobra.Command{ return err } - lsOutputs := make([]lsOutput, 0) + lsOutputs := make([]jsonDirEntry, 0) for _, entry := range entries { - parsedEntry, err := toLsOutput(entry) + parsedEntry, err := toJsonDirEntry(entry) if err != nil { return err } @@ -60,7 +79,10 @@ var lsCmd = &cobra.Command{ // Use template for long mode if the flag is set if longMode { - return cmdio.RenderWithTemplate(ctx, lsOutputs, "template_long") + return cmdio.RenderWithTemplate(ctx, lsOutputs, cmdio.Heredoc(` + {{range .}}{{if .IsDir}}DIRECTORY {{else}}FILE {{end}}{{.Size}} {{.ModTime|pretty_date}} {{.Name}} + {{end}} + `)) } return cmdio.Render(ctx, lsOutputs) }, diff --git a/cmd/fs/ls_output.go b/cmd/fs/ls_output.go deleted file mode 100644 index e8728b046..000000000 --- a/cmd/fs/ls_output.go +++ /dev/null @@ -1,27 +0,0 @@ -package fs - -import ( - "io/fs" - "time" -) - -type lsOutput struct { - Name string `json:"name"` - IsDir bool `json:"is_directory"` - Size int64 `json:"size"` - ModTime time.Time `json:"last_modified"` -} - -func toLsOutput(f fs.DirEntry) (*lsOutput, error) { - info, err := f.Info() - if err != nil { - return nil, err - } - - return &lsOutput{ - Name: f.Name(), - IsDir: f.IsDir(), - Size: info.Size(), - ModTime: info.ModTime(), - }, nil -} diff --git a/cmd/root/io.go b/cmd/root/io.go index 07db9e0a0..93830c804 100644 --- a/cmd/root/io.go +++ b/cmd/root/io.go @@ -2,7 +2,6 @@ package root import ( "os" - "strings" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/flags" @@ -28,14 +27,13 @@ func OutputType() flags.Output { } func initializeIO(cmd *cobra.Command) error { - templates := make(map[string]string, 0) - for k, v := range cmd.Annotations { - if strings.Contains(k, "template") { - templates[k] = v - } + var template string + if cmd.Annotations != nil { + // rely on zeroval being an empty string + template = cmd.Annotations["template"] } - cmdIO := cmdio.NewIO(outputType, cmd.InOrStdin(), cmd.OutOrStdout(), cmd.ErrOrStderr(), templates) + cmdIO := cmdio.NewIO(outputType, cmd.InOrStdin(), cmd.OutOrStdout(), cmd.ErrOrStderr(), template) ctx := cmdio.InContext(cmd.Context(), cmdIO) cmd.SetContext(ctx) diff --git a/libs/cmdio/io.go b/libs/cmdio/io.go index 4f134c510..1df6f5c18 100644 --- a/libs/cmdio/io.go +++ b/libs/cmdio/io.go @@ -24,17 +24,17 @@ type cmdIO struct { // e.g. if stdout is a terminal interactive bool outputFormat flags.Output - templates map[string]string + template string in io.Reader out io.Writer err io.Writer } -func NewIO(outputFormat flags.Output, in io.Reader, out io.Writer, err io.Writer, templates map[string]string) *cmdIO { +func NewIO(outputFormat flags.Output, in io.Reader, out io.Writer, err io.Writer, template string) *cmdIO { return &cmdIO{ interactive: !color.NoColor, outputFormat: outputFormat, - templates: templates, + template: template, in: in, out: out, err: err, @@ -66,14 +66,20 @@ func (c *cmdIO) IsTTY() bool { return isatty.IsTerminal(fd) || isatty.IsCygwinTerminal(fd) } -func (c *cmdIO) Render(v any, templateName string) error { +func Render(ctx context.Context, v any) error { + c := fromContext(ctx) + return RenderWithTemplate(ctx, v, c.template) +} + +func RenderWithTemplate(ctx context.Context, v any, template string) error { // TODO: add terminal width & white/dark theme detection + c := fromContext(ctx) switch c.outputFormat { case flags.OutputJSON: return renderJson(c.out, v) case flags.OutputText: - if c.templates[templateName] != "" { - return renderTemplate(c.out, c.templates[templateName], v) + if template != "" { + return renderTemplate(c.out, template, v) } return renderJson(c.out, v) default: @@ -81,16 +87,6 @@ func (c *cmdIO) Render(v any, templateName string) error { } } -func Render(ctx context.Context, v any) error { - c := fromContext(ctx) - return c.Render(v, "template") -} - -func RenderWithTemplate(ctx context.Context, v any, templateName string) error { - c := fromContext(ctx) - return c.Render(v, templateName) -} - type tuple struct{ Name, Id string } func (c *cmdIO) Select(names map[string]string, label string) (id string, err error) { From 8050dc524f0433b1eb9cf30854b318097a67f954 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 5 Jun 2023 13:53:38 +0200 Subject: [PATCH 2/7] move sort outside --- cmd/fs/ls.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 1df8dfcf4..43f0a4993 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -65,26 +65,26 @@ var lsCmd = &cobra.Command{ return err } - lsOutputs := make([]jsonDirEntry, 0) + jsonDirEntries := make([]jsonDirEntry, 0) for _, entry := range entries { - parsedEntry, err := toJsonDirEntry(entry) + jsonDirEntry, err := toJsonDirEntry(entry) if err != nil { return err } - lsOutputs = append(lsOutputs, *parsedEntry) - sort.Slice(lsOutputs, func(i, j int) bool { - return lsOutputs[i].Name < lsOutputs[j].Name - }) + jsonDirEntries = append(jsonDirEntries, *jsonDirEntry) } + sort.Slice(jsonDirEntries, func(i, j int) bool { + return jsonDirEntries[i].Name < jsonDirEntries[j].Name + }) // Use template for long mode if the flag is set if longMode { - return cmdio.RenderWithTemplate(ctx, lsOutputs, cmdio.Heredoc(` + return cmdio.RenderWithTemplate(ctx, jsonDirEntries, cmdio.Heredoc(` {{range .}}{{if .IsDir}}DIRECTORY {{else}}FILE {{end}}{{.Size}} {{.ModTime|pretty_date}} {{.Name}} {{end}} `)) } - return cmdio.Render(ctx, lsOutputs) + return cmdio.Render(ctx, jsonDirEntries) }, } From fc7fbbec6f86d80d8d655e224607fc55bab31a49 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 5 Jun 2023 13:55:29 +0200 Subject: [PATCH 3/7] - --- cmd/fs/ls.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 43f0a4993..c92393c08 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -91,6 +91,6 @@ var lsCmd = &cobra.Command{ var longMode bool func init() { - lsCmd.Flags().BoolVarP(&longMode, "long", "l", false, "Displays full information including size, file type and modification time since Epoch in milliseconds.") + lsCmd.Flags().BoolVarP(&longMode, "long", "l", false, "Displays full information including size, file type and modification time since Epoch in milliseconds.") fsCmd.AddCommand(lsCmd) } From b048c8675d9136fc70e3d2ceade892533ab2e67d Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 5 Jun 2023 14:01:06 +0200 Subject: [PATCH 4/7] address comments --- cmd/fs/helpers.go | 14 ++++++++++++++ .../fs/helpers_test.go | 18 +++++++++--------- cmd/fs/ls.go | 4 ++-- libs/filer/dbfs_client.go | 10 ---------- 4 files changed, 25 insertions(+), 21 deletions(-) create mode 100644 cmd/fs/helpers.go rename libs/filer/dbfs_client_test.go => cmd/fs/helpers_test.go (65%) diff --git a/cmd/fs/helpers.go b/cmd/fs/helpers.go new file mode 100644 index 000000000..e456bff98 --- /dev/null +++ b/cmd/fs/helpers.go @@ -0,0 +1,14 @@ +package fs + +import ( + "fmt" + "strings" +) + +func resolveDbfsPath(path string) (string, error) { + if !strings.HasPrefix(path, "dbfs:/") { + return "", fmt.Errorf("expected dbfs path (with the dbfs:/ prefix): %s", path) + } + + return strings.TrimPrefix(path, "dbfs:"), nil +} diff --git a/libs/filer/dbfs_client_test.go b/cmd/fs/helpers_test.go similarity index 65% rename from libs/filer/dbfs_client_test.go rename to cmd/fs/helpers_test.go index e9d1f136b..1d174ef95 100644 --- a/libs/filer/dbfs_client_test.go +++ b/cmd/fs/helpers_test.go @@ -1,4 +1,4 @@ -package filer +package fs import ( "testing" @@ -7,32 +7,32 @@ import ( ) func TestResolveDbfsPath(t *testing.T) { - path, err := ResolveDbfsPath("dbfs:/") + path, err := resolveDbfsPath("dbfs:/") assert.NoError(t, err) assert.Equal(t, "/", path) - path, err = ResolveDbfsPath("dbfs:/abc") + path, err = resolveDbfsPath("dbfs:/abc") assert.NoError(t, err) assert.Equal(t, "/abc", path) - path, err = ResolveDbfsPath("dbfs:/a/b/c") + path, err = resolveDbfsPath("dbfs:/a/b/c") assert.NoError(t, err) assert.Equal(t, "/a/b/c", path) - path, err = ResolveDbfsPath("dbfs:/a/b/.") + path, err = resolveDbfsPath("dbfs:/a/b/.") assert.NoError(t, err) assert.Equal(t, "/a/b/.", path) - path, err = ResolveDbfsPath("dbfs:/a/../c") + path, err = resolveDbfsPath("dbfs:/a/../c") assert.NoError(t, err) assert.Equal(t, "/a/../c", path) - _, err = ResolveDbfsPath("dbf:/a/b/c") + _, err = resolveDbfsPath("dbf:/a/b/c") assert.ErrorContains(t, err, "expected dbfs path (with the dbfs:/ prefix): dbf:/a/b/c") - _, err = ResolveDbfsPath("/a/b/c") + _, err = resolveDbfsPath("/a/b/c") assert.ErrorContains(t, err, "expected dbfs path (with the dbfs:/ prefix): /a/b/c") - _, err = ResolveDbfsPath("dbfs:a/b/c") + _, err = resolveDbfsPath("dbfs:a/b/c") assert.ErrorContains(t, err, "expected dbfs path (with the dbfs:/ prefix): dbfs:a/b/c") } diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index c92393c08..7049db049 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -34,7 +34,7 @@ func toJsonDirEntry(f fs.DirEntry) (*jsonDirEntry, error) { // lsCmd represents the ls command var lsCmd = &cobra.Command{ - Use: "ls ", + Use: "ls DIR_PATH", Short: "Lists files", Long: `Lists files`, Args: cobra.ExactArgs(1), @@ -50,7 +50,7 @@ var lsCmd = &cobra.Command{ ctx := cmd.Context() w := root.WorkspaceClient(ctx) - path, err := filer.ResolveDbfsPath(args[0]) + path, err := resolveDbfsPath(args[0]) if err != nil { return err } diff --git a/libs/filer/dbfs_client.go b/libs/filer/dbfs_client.go index d1cf9270c..67878136b 100644 --- a/libs/filer/dbfs_client.go +++ b/libs/filer/dbfs_client.go @@ -3,13 +3,11 @@ package filer import ( "context" "errors" - "fmt" "io" "io/fs" "net/http" "path" "sort" - "strings" "time" "github.com/databricks/databricks-sdk-go" @@ -272,11 +270,3 @@ func (w *DbfsClient) Stat(ctx context.Context, name string) (fs.FileInfo, error) return dbfsFileInfo{*info}, nil } - -func ResolveDbfsPath(path string) (string, error) { - if !strings.HasPrefix(path, "dbfs:/") { - return "", fmt.Errorf("expected dbfs path (with the dbfs:/ prefix): %s", path) - } - - return strings.TrimPrefix(path, "dbfs:"), nil -} From 265e517126d08eea462bce16b032b839ca7d3847 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 5 Jun 2023 15:07:08 +0200 Subject: [PATCH 5/7] address comments 2 --- cmd/fs/ls.go | 11 ++++------- libs/cmdio/render.go | 2 +- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 7049db049..2e1e4f61f 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -39,12 +39,6 @@ var lsCmd = &cobra.Command{ Long: `Lists files`, Args: cobra.ExactArgs(1), PreRunE: root.MustWorkspaceClient, - Annotations: map[string]string{ - "template": cmdio.Heredoc(` - {{range .}}{{.Name}} - {{end}} - `), - }, RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() @@ -84,7 +78,10 @@ var lsCmd = &cobra.Command{ {{end}} `)) } - return cmdio.Render(ctx, jsonDirEntries) + return cmdio.RenderWithTemplate(ctx, jsonDirEntries, cmdio.Heredoc(` + {{range .}}{{.Name}} + {{end}} + `)) }, } diff --git a/libs/cmdio/render.go b/libs/cmdio/render.go index 063d7cbcb..2e42c32c2 100644 --- a/libs/cmdio/render.go +++ b/libs/cmdio/render.go @@ -88,7 +88,7 @@ func renderTemplate(w io.Writer, tmpl string, v any) error { return string(b), nil }, "pretty_date": func(t time.Time) string { - return t.UTC().Format("2006-01-02T15:04:05Z") + return t.Format("2006-01-02T15:04:05Z") }, }).Parse(tmpl) if err != nil { From 08826094ce10142cd0524bfd0eed47612aa6c842 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 5 Jun 2023 15:09:09 +0200 Subject: [PATCH 6/7] added preallocation of size --- cmd/fs/ls.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 2e1e4f61f..85e3ffdfa 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -59,13 +59,13 @@ var lsCmd = &cobra.Command{ return err } - jsonDirEntries := make([]jsonDirEntry, 0) - for _, entry := range entries { + jsonDirEntries := make([]jsonDirEntry, len(entries)) + for i, entry := range entries { jsonDirEntry, err := toJsonDirEntry(entry) if err != nil { return err } - jsonDirEntries = append(jsonDirEntries, *jsonDirEntry) + jsonDirEntries[i] = *jsonDirEntry } sort.Slice(jsonDirEntries, func(i, j int) bool { return jsonDirEntries[i].Name < jsonDirEntries[j].Name From 9160a6e9ed2566b0b4d36a5388f0f70563509173 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 5 Jun 2023 15:15:21 +0200 Subject: [PATCH 7/7] initialize dbfs client at root --- cmd/fs/ls.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/fs/ls.go b/cmd/fs/ls.go index 85e3ffdfa..200cbed52 100644 --- a/cmd/fs/ls.go +++ b/cmd/fs/ls.go @@ -49,12 +49,12 @@ var lsCmd = &cobra.Command{ return err } - f, err := filer.NewDbfsClient(w, path) + f, err := filer.NewDbfsClient(w, "/") if err != nil { return err } - entries, err := f.ReadDir(ctx, "") + entries, err := f.ReadDir(ctx, path) if err != nil { return err }