From 6d3dce1feec04238e5459386baeb8b69838c4769 Mon Sep 17 00:00:00 2001 From: Adam Babik Date: Sat, 10 Feb 2024 10:11:22 +0100 Subject: [PATCH] Remove some TODOs (#491) --- internal/cmd/print.go | 4 +- internal/cmd/run.go | 60 +++++++++++- internal/cmd/tasks.go | 3 +- internal/cmd/tui.go | 2 +- internal/document/document.go | 7 +- internal/document/frontmatter.go | 10 +- internal/document/parser.go | 8 +- internal/document/parser_frontmatter.go | 12 +-- internal/project/project.go | 27 ++---- internal/project/project_test.go | 3 +- internal/project/task.go | 124 +++++++++--------------- internal/version/version.go | 6 ++ internal/version/version_test.go | 78 +++++++++++---- 13 files changed, 200 insertions(+), 144 deletions(-) diff --git a/internal/cmd/print.go b/internal/cmd/print.go index aad1e2e22..5f4a074a0 100644 --- a/internal/cmd/print.go +++ b/internal/cmd/print.go @@ -5,8 +5,6 @@ import ( "github.com/pkg/errors" "github.com/spf13/cobra" - - "github.com/stateful/runme/internal/project" ) func printCmd() *cobra.Command { @@ -24,7 +22,7 @@ func printCmd() *cobra.Command { } task, err := lookupTaskWithPrompt(cmd, args[0], tasks) - if project.IsTaskNotFoundError(err) && !fAllowUnnamed { + if isTaskNotFoundError(err) && !fAllowUnnamed { fAllowUnnamed = true goto generateBlocks } else if err != nil { diff --git a/internal/cmd/run.go b/internal/cmd/run.go index c2ea69401..00a939c3f 100644 --- a/internal/cmd/run.go +++ b/internal/cmd/run.go @@ -114,7 +114,7 @@ func runCmd() *cobra.Command { for _, arg := range args { task, err := lookupTaskWithPrompt(cmd, arg, tasks) if err != nil { - if project.IsTaskNotFoundError(err) && !fAllowUnnamed { + if isTaskNotFoundError(err) && !fAllowUnnamed { fAllowUnnamed = true goto searchBlocks } @@ -461,13 +461,69 @@ func (p RunBlockPrompt) View() string { return blockPromptAppStyle.Render(content) } +type errTaskWithFilenameNotFound struct { + queryFile string +} + +func (e errTaskWithFilenameNotFound) Error() string { + return fmt.Sprintf("unable to find file in project matching regex %q", e.queryFile) +} + +type errTaskWithNameNotFound struct { + queryName string +} + +func (e errTaskWithNameNotFound) Error() string { + return fmt.Sprintf("unable to find any script named %q", e.queryName) +} + +func isTaskNotFoundError(err error) bool { + return errors.As(err, &errTaskWithFilenameNotFound{}) || errors.As(err, &errTaskWithNameNotFound{}) +} + +func filterTasksByFileAndTaskName(tasks []project.Task, queryFile, queryName string) ([]project.Task, error) { + fileMatcher, err := project.CompileRegex(queryFile) + if err != nil { + return nil, err + } + + var results []project.Task + + foundFile := false + + for _, task := range tasks { + if !fileMatcher.MatchString(task.DocumentPath) { + continue + } + + foundFile = true + + // This is expected that the task name query is + // matched exactly. + if queryName != task.CodeBlock.Name() { + continue + } + + results = append(results, task) + } + + if len(results) == 0 { + if !foundFile { + return nil, &errTaskWithFilenameNotFound{queryFile: queryFile} + } + return nil, &errTaskWithNameNotFound{queryName: queryName} + } + + return results, nil +} + func lookupTaskWithPrompt(cmd *cobra.Command, query string, tasks []project.Task) (task project.Task, err error) { queryFile, queryName, err := splitRunArgument(query) if err != nil { return task, err } - filteredTasks, err := project.FilterTasksByFileAndTaskName(tasks, queryFile, queryName) + filteredTasks, err := filterTasksByFileAndTaskName(tasks, queryFile, queryName) if err != nil { return task, err } diff --git a/internal/cmd/tasks.go b/internal/cmd/tasks.go index ad93a970f..4f090cd53 100644 --- a/internal/cmd/tasks.go +++ b/internal/cmd/tasks.go @@ -5,7 +5,6 @@ import ( "github.com/pkg/errors" "github.com/spf13/cobra" - "github.com/stateful/runme/internal/project" "github.com/stateful/runme/internal/tasks" ) @@ -24,7 +23,7 @@ func tasksCmd() *cobra.Command { task, err := lookupTaskWithPrompt(cmd, args[0], projTasks) if err != nil { - if project.IsTaskNotFoundError(err) && !fAllowUnnamed { + if isTaskNotFoundError(err) && !fAllowUnnamed { fAllowUnnamed = true goto generateBlocks } diff --git a/internal/cmd/tui.go b/internal/cmd/tui.go index e902bb680..72f21f5c5 100644 --- a/internal/cmd/tui.go +++ b/internal/cmd/tui.go @@ -266,7 +266,7 @@ func (m *tuiModel) filterCodeBlocks() { oldSelection = m.tasks[m.cursor] } - m.tasks, _ = project.FilterTasks(m.unfilteredTasks, func(t project.Task) (bool, error) { + m.tasks, _ = project.FilterTasksByFn(m.unfilteredTasks, func(t project.Task) (bool, error) { if !m.allowUnknown && t.CodeBlock.IsUnknown() { return false, nil } diff --git a/internal/document/document.go b/internal/document/document.go index 60093a8c4..57bde6a70 100644 --- a/internal/document/document.go +++ b/internal/document/document.go @@ -122,11 +122,12 @@ func (d *Document) splitSource() { d.content = item.Value(d.source) d.contentOffset = item.start case parsedItemError: - // TODO(adamb): handle this error somehow - if !errors.Is(item.err, errParseFrontmatter) { + if errors.Is(item.err, errParseRawFrontmatter) { + d.parseFrontmatterErr = item.err + } else { d.splitSourceErr = item.err - return } + return } } }) diff --git a/internal/document/frontmatter.go b/internal/document/frontmatter.go index 919a190f3..8b6f5b2f4 100644 --- a/internal/document/frontmatter.go +++ b/internal/document/frontmatter.go @@ -144,17 +144,11 @@ func (f *Frontmatter) ensureID() { f.Runme.ID = ulid.GenerateID() } - baseVersion := version.BaseVersion() - if baseVersion == "v99.9" || baseVersion == "v0.0" { - return + if v, ok := version.BaseVersionAuthoritative(); ok { + f.Runme.Version = v } - f.Runme.Version = baseVersion } -// TODO(adamb): Frontmatter can return (nil, nil) which indicates that -// there is no error, but also that there is no FrontMatter. It is not -// the best API. Consider adding HasFrontmatter() and returning an error -// if there is none from this method. func (d *Document) Frontmatter() (*Frontmatter, error) { d.splitSource() diff --git a/internal/document/parser.go b/internal/document/parser.go index 7901a1316..3d74d8c48 100644 --- a/internal/document/parser.go +++ b/internal/document/parser.go @@ -25,7 +25,7 @@ func ParseSections(source []byte) (result ParsedSections, _ error) { result.ContentOffset = item.start result.Content = item.Value(source) case parsedItemError: - if errors.Is(item.err, errParseFrontmatter) { + if errors.Is(item.err, errParseRawFrontmatter) { return ParsedSections{ Content: source, }, nil @@ -155,11 +155,11 @@ loop: switch { case r == '+': - return parseFrontMatter(l, byte(r)) + return parseRawFrontmatter(l, byte(r)) case r == '-': - return parseFrontMatter(l, byte(r)) + return parseRawFrontmatter(l, byte(r)) case r == '{': - return parseFrontMatterJSON + return parseRawFrontmatterJSON case r == '\ufeff': // skip case !unicode.IsSpace(r) && !isEOL(r): diff --git a/internal/document/parser_frontmatter.go b/internal/document/parser_frontmatter.go index a352d7a4d..30622a799 100644 --- a/internal/document/parser_frontmatter.go +++ b/internal/document/parser_frontmatter.go @@ -5,14 +5,14 @@ import ( "errors" ) -var errParseFrontmatter = errors.New("failed to parse frontmatter") +var errParseRawFrontmatter = errors.New("failed to extract frontmatter") -func parseFrontMatter(l *itemParser, delimiter byte) parserStateFunc { - // lexFrontMatter was trigger when a dilimiter was observer. - // According to the spec, there must be three delimiter characters. +// parseRawFrontmatter is a parser state function that extracts the frontmatter with +// "---" or "+++" delimiters. According to the spec, there must be three delimiter characters. +func parseRawFrontmatter(l *itemParser, delimiter byte) parserStateFunc { for i := 0; i < 2; i++ { if r := l.next(); r != rune(delimiter) { - l.error(errParseFrontmatter) + l.error(errParseRawFrontmatter) return nil } } @@ -43,7 +43,7 @@ func parseFrontMatter(l *itemParser, delimiter byte) parserStateFunc { return parseContent } -func parseFrontMatterJSON(l *itemParser) parserStateFunc { +func parseRawFrontmatterJSON(l *itemParser) parserStateFunc { l.backup() var ( diff --git a/internal/project/project.go b/internal/project/project.go index e73c6539f..c5d032b14 100644 --- a/internal/project/project.go +++ b/internal/project/project.go @@ -6,7 +6,6 @@ import ( "io/fs" "os" "path/filepath" - "reflect" "strings" "github.com/go-git/go-billy/v5" @@ -69,17 +68,11 @@ type LoadEvent struct { Data any } -// TODO(adamb): add more robust implementation. -// -// Consider switching away from reflection -// as this method is used in hot code path. -func (e LoadEvent) extractDataValue(val any) { - reflect.ValueOf(val).Elem().Set(reflect.ValueOf(e.Data)) -} - func ExtractDataFromLoadEvent[T any](event LoadEvent) T { - var data T - event.extractDataValue(&data) + data, ok := event.Data.(T) + if !ok { + panic("invariant: incompatible types") + } return data } @@ -534,12 +527,12 @@ func (p *Project) LoadEnv() ([]string, error) { } func (p *Project) LoadEnvAsMap() (map[string]string, error) { - // For file-based projects, there are no envs to read. + // For file-based projects, there are no env to read. if p.fs == nil { return nil, nil } - envs := make(map[string]string) + env := make(map[string]string) for _, envFile := range p.envFilesReadOrder { bytes, err := util.ReadFile(p.fs, envFile) @@ -555,15 +548,13 @@ func (p *Project) LoadEnvAsMap() (map[string]string, error) { parsed, err := godotenv.UnmarshalBytes(bytes) if err != nil { - // silently fail for now - // TODO(mxs): come up with better solution - continue + return nil, errors.WithStack(err) } for k, v := range parsed { - envs[k] = v + env[k] = v } } - return envs, nil + return env, nil } diff --git a/internal/project/project_test.go b/internal/project/project_test.go index 1ec5d7755..43dfdcd1d 100644 --- a/internal/project/project_test.go +++ b/internal/project/project_test.go @@ -6,9 +6,10 @@ import ( "path/filepath" "testing" - "github.com/stateful/runme/internal/project/testdata" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/stateful/runme/internal/project/testdata" ) func TestExtractDataFromLoadEvent(t *testing.T) { diff --git a/internal/project/task.go b/internal/project/task.go index c46b030c6..8e0cc94ca 100644 --- a/internal/project/task.go +++ b/internal/project/task.go @@ -33,10 +33,6 @@ func (t Task) Format(s fmt.State, verb rune) { } // LoadFiles returns a list of file names found in the project. -// -// TODO(adamb): figure out a strategy for error handling. There are -// two options: (1) stop and return the error immediately, -// (2) continue and return first (or all) error and the list of file names. func LoadFiles(ctx context.Context, p *Project) ([]string, error) { eventc := make(chan LoadEvent) @@ -89,11 +85,28 @@ func LoadTasks(ctx context.Context, p *Project) ([]Task, error) { return result, err } +type Filter func(Task) (bool, error) + var ErrReturnEarly = errors.New("return early") -func FilterTasks(tasks []Task, fn func(Task) (bool, error)) (result []Task, err error) { +func FilterTasksByFn(tasks []Task, fns ...Filter) (result []Task, err error) { for _, task := range tasks { - include, errFn := fn(task) + var errFn error + + include := true + + for _, fn := range fns { + var ok bool + + ok, errFn = fn(task) + if !ok { + include = false + break + } + if errFn != nil { + break + } + } if include { result = append(result, task) @@ -103,100 +116,57 @@ func FilterTasks(tasks []Task, fn func(Task) (bool, error)) (result []Task, err if !errors.Is(errFn, ErrReturnEarly) { err = errFn } - break + return } } return } -type Matcher interface { - MatchString(string) bool -} - -type matchAll struct{} - -func (matchAll) MatchString(string) bool { return true } - -func compileQuery(query string) (Matcher, error) { - if query == "" { - return &matchAll{}, nil - } - reg, err := regexp.Compile(query) - return reg, errors.Wrapf(err, "invalid regexp %q", query) -} - -func FilterTasksByID(tasks []Task, query string) ([]Task, error) { - queryMatcher, err := compileQuery(query) +func FilterTasksByID(tasks []Task, expr string) (result []Task, err error) { + matcher, err := compileRegex(expr) if err != nil { return nil, err } - var results []Task - - for _, task := range tasks { - if !queryMatcher.MatchString(task.ID()) { - continue - } - - results = append(results, task) - } - - return results, nil + return FilterTasksByFn(tasks, func(task Task) (bool, error) { + return matcher.MatchString(task.ID()), nil + }) } -func FilterTasksByFileAndTaskName(tasks []Task, queryFile string, queryName string) ([]Task, error) { - fileMatcher, err := compileQuery(queryFile) +func FilterTasksByFilename(tasks []Task, expr string) (result []Task, err error) { + matcher, err := compileRegex(expr) if err != nil { return nil, err } - var results []Task - - foundFile := false - - for _, task := range tasks { - if !fileMatcher.MatchString(task.DocumentPath) { - continue - } - - foundFile = true - - // This is expected that the task name query is - // matched exactly. - if queryName != task.CodeBlock.Name() { - continue - } - - results = append(results, task) - } - - if len(results) == 0 { - if !foundFile { - return nil, &ErrTaskWithFilenameNotFound{queryFile: queryFile} - } - return nil, &ErrTaskWithNameNotFound{queryName: queryName} - } - - return results, nil + return FilterTasksByFn(tasks, func(task Task) (bool, error) { + return matcher.MatchString(task.DocumentPath), nil + }) } -type ErrTaskWithFilenameNotFound struct { - queryFile string +func FilterTasksByExactTaskName(tasks []Task, name string) (result []Task, err error) { + return FilterTasksByFn(tasks, func(task Task) (bool, error) { + return task.CodeBlock.Name() == name, nil + }) } -func (e ErrTaskWithFilenameNotFound) Error() string { - return fmt.Sprintf("unable to find file in project matching regex %q", e.queryFile) +func CompileRegex(query string) (Matcher, error) { + return compileRegex(query) } -type ErrTaskWithNameNotFound struct { - queryName string +func compileRegex(query string) (Matcher, error) { + if query == "" { + return &matchAll{}, nil + } + reg, err := regexp.Compile(query) + return reg, errors.Wrapf(err, "invalid regexp %q", query) } -func (e ErrTaskWithNameNotFound) Error() string { - return fmt.Sprintf("unable to find any script named %q", e.queryName) +type Matcher interface { + MatchString(string) bool } -func IsTaskNotFoundError(err error) bool { - return errors.As(err, &ErrTaskWithFilenameNotFound{}) || errors.As(err, &ErrTaskWithNameNotFound{}) -} +type matchAll struct{} + +func (matchAll) MatchString(string) bool { return true } diff --git a/internal/version/version.go b/internal/version/version.go index 5bca37e24..4e6cd5fef 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -21,3 +21,9 @@ func BaseVersion() string { return fmt.Sprintf("v%d.%d", v.Major(), v.Minor()) } + +func BaseVersionAuthoritative() (string, bool) { + _, err := semver.NewVersion(BuildVersion) + baseVersion := BaseVersion() + return baseVersion, err == nil && baseVersion != "v0.0" && baseVersion != "v99.9" +} diff --git a/internal/version/version_test.go b/internal/version/version_test.go index c751513f6..fc60b5dad 100644 --- a/internal/version/version_test.go +++ b/internal/version/version_test.go @@ -8,34 +8,34 @@ import ( func TestBaseVersion(t *testing.T) { tests := []struct { - name string - buildVersion string - expectedMinor string + name string + buildVersion string + expectedBaseVersion string }{ { - name: "standard version", - buildVersion: "1.7.8-11-g2300850-2300850", - expectedMinor: "v1.7", + name: "standard version", + buildVersion: "1.7.8-11-g2300850-2300850", + expectedBaseVersion: "v1.7", }, { - name: "only major and minor version", - buildVersion: "2.3-11-g2300850-2300850", - expectedMinor: "v2.3", + name: "only major and minor version", + buildVersion: "2.3-11-g2300850-2300850", + expectedBaseVersion: "v2.3", }, { - name: "only major version", - buildVersion: "3-11-g2300850-2300850", - expectedMinor: "v3.0", + name: "only major version", + buildVersion: "3-11-g2300850-2300850", + expectedBaseVersion: "v3.0", }, { - name: "no version", - buildVersion: "0.0.0", - expectedMinor: "v0.0", + name: "no version", + buildVersion: "0.0.0", + expectedBaseVersion: "v0.0", }, { - name: "invalid semver", - buildVersion: "1.2.beta", - expectedMinor: "unknown", + name: "invalid semver", + buildVersion: "1.2.beta", + expectedBaseVersion: "unknown", }, } @@ -43,7 +43,47 @@ func TestBaseVersion(t *testing.T) { t.Run(tt.name, func(t *testing.T) { BuildVersion = tt.buildVersion baseVersion := BaseVersion() - assert.Equal(t, tt.expectedMinor, baseVersion) + assert.Equal(t, tt.expectedBaseVersion, baseVersion) + }) + } +} + +func TestBaseVersionAuthoritative(t *testing.T) { + tests := []struct { + name string + buildVersion string + expectedBaseVersion string + authoritative bool + }{ + { + name: "zeros", + buildVersion: "0.0.0", + expectedBaseVersion: "v0.0", + }, + { + name: "nines", + buildVersion: "99.9.9", + expectedBaseVersion: "v99.9", + }, + { + name: "standard version", + buildVersion: "1.7.8-11-g2300850-2300850", + expectedBaseVersion: "v1.7", + authoritative: true, + }, + { + name: "invalid semver", + buildVersion: "1.2.beta", + expectedBaseVersion: "unknown", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + BuildVersion = tt.buildVersion + baseVersion, authoritative := BaseVersionAuthoritative() + assert.Equal(t, tt.expectedBaseVersion, baseVersion) + assert.Equal(t, tt.authoritative, authoritative) }) } }