diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 87aa808..b27d3cc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -27,7 +27,7 @@ cmd/book/ pkg/book/ ├── types.go # Core data structs: Config, BookShelves, Shelf, Collection, Mark ├── doctor.go # MarkConflict, DetectDuplicates, ResolveDuplicates -└── templates.go # ViewTemplate, DefaultViewTemplates, Templatable helpers +└── templates.go # ViewTemplate, DefaultViewTemplates pkg/catalog/ ├── catalog.go # Paths, ShelfPath, VerifyExists, LoadShelves diff --git a/README.md b/README.md index 600ef60..b40f40e 100644 --- a/README.md +++ b/README.md @@ -222,7 +222,7 @@ paths := catalog.Paths{ShelfRoot: "/path/to/shelf.d", CatalogFormat: "toml"} shelf, _ := book.NewShelf("work", "work stuff") collection, _ := book.NewCollection(shelf, "golang", "go links") mark, _ := book.NewMarkFromInput("https://go.dev", book.SplitTags("lang,official")) -mark.Name = "The Go Programming Language" +mark.Title = "The Go Programming Language" mark.Shelf = shelf mark.Collection = collection mark.RecordAdd() diff --git a/VERSION b/VERSION index 0236045..a20e2d8 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -v1.6.1 +v1.7.0 diff --git a/cmd/book/actions_test.go b/cmd/book/actions_test.go index 89be990..7f3ae03 100644 --- a/cmd/book/actions_test.go +++ b/cmd/book/actions_test.go @@ -2,6 +2,8 @@ package cmd import ( "context" + "errors" + "fmt" "os" "path/filepath" "strings" @@ -10,6 +12,7 @@ import ( "github.com/polymorcodeus/book/internal/theme" "github.com/polymorcodeus/book/pkg/book" "github.com/polymorcodeus/book/pkg/catalog" + "github.com/polymorcodeus/book/pkg/web" ) func testConfig(t *testing.T) *theme.UIConfig { @@ -181,8 +184,8 @@ func TestAddMark(t *testing.T) { t.Fatalf("got %d marks, want 1", len(collection.Marks)) } mark := collection.Marks[0] - if mark.Name != "Example" { - t.Errorf("Name = %q, want Example", mark.Name) + if mark.Title != "Example" { + t.Errorf("Title = %q, want Example", mark.Title) } if !strings.EqualFold(strings.Join(mark.Tags, ","), "go,cli") { t.Errorf("Tags = %v, want [go cli]", mark.Tags) @@ -239,8 +242,8 @@ func TestEditMark(t *testing.T) { reloaded := loadShelves(t, config) updated := testShelf(t, reloaded, "dev").Collection("docs").Marks[0] - if updated.Name != "Updated" { - t.Errorf("Name = %q, want Updated", updated.Name) + if updated.Title != "Updated" { + t.Errorf("Title = %q, want Updated", updated.Title) } if !strings.EqualFold(strings.Join(updated.Tags, ","), "go,cli") { t.Errorf("Tags = %v, want [go cli]", updated.Tags) @@ -324,3 +327,71 @@ func TestRequireFlags(t *testing.T) { } } } + +func TestWebsiteError(t *testing.T) { + notFound := websiteError(fmt.Errorf("fetch title: %w: %s", web.ErrNotFound, "https://example.com"), "https://example.com") + if !strings.Contains(notFound.Error(), "4oh4") { + t.Errorf("websiteError(ErrNotFound) = %q, want the 4oh4 quip", notFound) + } + if !strings.Contains(notFound.Error(), "https://example.com") { + t.Errorf("websiteError(ErrNotFound) = %q, want the url", notFound) + } + + other := websiteError(errors.New("connection refused"), "https://example.com") + if strings.Contains(other.Error(), "4oh4") { + t.Errorf("websiteError(other) = %q, want a plain wrapped error", other) + } + if !strings.Contains(other.Error(), "load website:") { + t.Errorf("websiteError(other) = %q, want the load website context", other) + } +} + +func TestUniqueURLError(t *testing.T) { + trashed := &book.Mark{ID: "abc12345", Title: "gone", URL: "https://example.com/trashed", DeletedAt: book.NowTimestamp()} + active := &book.Mark{ID: "def67890", Title: "here", URL: "https://example.com/active"} + bs := book.BookShelves{{ + Name: "dev", + Collections: map[string]*book.Collection{ + "docs": {Name: "docs", Marks: []*book.Mark{trashed, active}}, + }, + }} + + trashedErr := bs.VerifyUniqueURL("abc12345", nil) + if trashedErr == nil { + t.Fatal("VerifyUniqueURL(trashed) expected error") + } + got := uniqueURLError(&bs, "abc12345", trashedErr) + if !strings.Contains(got.Error(), "book mark restore --id abc12345") { + t.Errorf("uniqueURLError(trashed) = %q, want the restore command with id", got) + } + if !errors.Is(got, book.ErrURLTrashed) { + t.Errorf("uniqueURLError(trashed) lost the ErrURLTrashed chain: %v", got) + } + + dupErr := bs.VerifyUniqueURL("def67890", nil) + if dupErr == nil { + t.Fatal("VerifyUniqueURL(active) expected error") + } + got = uniqueURLError(&bs, "def67890", dupErr) + if strings.Contains(got.Error(), "restore") { + t.Errorf("uniqueURLError(active) = %q, want no restore hint", got) + } + if !errors.Is(got, book.ErrDuplicateURL) { + t.Errorf("uniqueURLError(active) lost the ErrDuplicateURL chain: %v", got) + } +} + +func TestTitleError(t *testing.T) { + required := titleError(fmt.Errorf("%w for %s", book.ErrTitleRequired, "https://example.com")) + if !strings.Contains(required.Error(), "--title") { + t.Errorf("titleError(ErrTitleRequired) = %q, want the --title hint", required) + } + if !errors.Is(required, book.ErrTitleRequired) { + t.Errorf("titleError(ErrTitleRequired) lost the chain: %v", required) + } + + other := errors.New("boom") + if got := titleError(other); got.Error() != "boom" { + t.Errorf("titleError(other) = %q, want the error unchanged", got) + } +} diff --git a/cmd/book/doctor.go b/cmd/book/doctor.go index 44d522a..1b721ee 100644 --- a/cmd/book/doctor.go +++ b/cmd/book/doctor.go @@ -132,7 +132,7 @@ func printDoctorReport(r doctorReport) { fmt.Printf(" %s %s\n", c.ID, c.URL) for _, m := range c.Marks { fmt.Printf(" - %s / %s title=%q tags=%v deleted=%t\n", - m.Shelf.Name, m.Collection.Name, m.Name, m.Tags, m.IsDeleted()) + m.Shelf.Name, m.Collection.Name, m.Title, m.Tags, m.IsDeleted()) } } fmt.Println() diff --git a/cmd/book/mark.go b/cmd/book/mark.go index 639a5af..8a00d91 100644 --- a/cmd/book/mark.go +++ b/cmd/book/mark.go @@ -79,7 +79,7 @@ func marks(cache *indexCache, bs *book.BookShelves, shelfName string, collection if m.IsDeleted() { continue } - fmt.Printf("%s %s\n", m.Name, m.URL) + fmt.Printf("%s %s\n", m.Title, m.URL) } return nil } @@ -109,12 +109,13 @@ func editMark(bs *book.BookShelves, id, title, tags, url string, config *theme.U if err := book.ValidateURL(url); err != nil { return err } - if err := bs.VerifyUniqueURL(book.GenerateID(url), target); err != nil { - return fmt.Errorf("verify unique url: %w", err) + id := book.GenerateID(url) + if err := bs.VerifyUniqueURL(id, target); err != nil { + return uniqueURLError(bs, id, err) } } - if err := target.UpdateMark(title, url, book.SplitTags(tags)); err != nil { + if err := target.Update(title, url, book.SplitTags(tags)); err != nil { return err } target.Touch() @@ -150,6 +151,33 @@ func searchMarks(cache *indexCache, query string, tags string, shelfName string, } } +func websiteError(err error, url string) error { + if errors.Is(err, web.ErrNotFound) { + return fmt.Errorf("betta check yerself - that's a 4oh4!\n%s", url) + } + return fmt.Errorf("load website: %w", err) +} + +// uniqueURLError annotates a VerifyUniqueURL failure with the restore command +// when the collision is with a trashed mark. +func uniqueURLError(bs *book.BookShelves, id string, err error) error { + if errors.Is(err, book.ErrURLTrashed) { + if trashed := bs.SoftDeletedByID(id); trashed != nil { + return fmt.Errorf("verify unique url: %w\n\nrestore it with:\nbook mark restore --id %s", err, trashed.ID) + } + } + return fmt.Errorf("verify unique url: %w", err) +} + +// titleError annotates a ResolveMarkTitle failure with the flag a +// non-interactive caller needs to supply the title manually. +func titleError(err error) error { + if errors.Is(err, book.ErrTitleRequired) { + return fmt.Errorf("%w; provide --title", err) + } + return err +} + func addMark(ctx context.Context, bs *book.BookShelves, URL string, tags string, shelfName string, collectionName string, title string, config *theme.UIConfig) error { mark, err := book.NewMarkFromInput(URL, book.SplitTags(tags)) if err != nil { @@ -158,7 +186,7 @@ func addMark(ctx context.Context, bs *book.BookShelves, URL string, tags string, // Ensure URL hash not in bookshelves if err := bs.VerifyUniqueURL(mark.ID, nil); err != nil { - return fmt.Errorf("verify unique url: %w", err) + return uniqueURLError(bs, mark.ID, err) } // Use provided title or fetch from URL @@ -166,17 +194,18 @@ func addMark(ctx context.Context, bs *book.BookShelves, URL string, tags string, if title == "" { fetchedTitle, err := loadWebsite(ctx, mark.URL) if err != nil { - if !errors.Is(err, web.ErrTitleUnavailable) { - return fmt.Errorf("load website: %w", err) + if errors.Is(err, web.ErrTitleUnavailable) { + fetched.Unavailable = true + } else { + return websiteError(err, mark.URL) } - fetched.Unavailable = true } else { fetched.Title = fetchedTitle } } - mark.Name, err = book.ResolveMarkTitle(title, mark.URL, fetched, config.Interactive) + mark.Title, err = book.ResolveMarkTitle(title, mark.URL, fetched, config.Interactive) if err != nil { - return err + return titleError(err) } // Non-interactive path: all required flags provided diff --git a/internal/model/collection_model.go b/internal/model/collection_model.go index 6e48f40..fc34e20 100644 --- a/internal/model/collection_model.go +++ b/internal/model/collection_model.go @@ -124,7 +124,10 @@ func (m getCollectionModel) ResultView() string { if m.get.book.form.State != huh.StateCompleted || m.get.book.err != nil || m.action != "list" { return "" } - return renderCompletedView(m.get.book.styles, m.get.book.tmpls, "collection-list", m.get.shelf).Content + return renderCompletedView(m.get.book.styles, m.get.book.tmpls, "collection-list", viewData{ + Primary: m.get.shelf.Name, + List: m.get.shelf.CollectionNames(), + }).Content } // Error returns the terminal error, if any, for the caller to surface after @@ -166,7 +169,7 @@ func GetCollectionForm(bs *book.BookShelves, config *theme.UIConfig, action stri if shelf == nil { return []huh.Option[string]{} } - return huh.NewOptions(shelf.CollectionsNames()...) + return huh.NewOptions(shelf.CollectionNames()...) }, &chosenShelf). Key("collection"). Value(&chosenCollection), @@ -311,7 +314,10 @@ func (m editCollectionModel) ResultView() string { if m.editor.book.form.State != huh.StateCompleted || m.editor.book.err != nil { return "" } - return renderCompletedView(m.editor.book.styles, m.editor.book.tmpls, "collection-add", m.editor.collection).Content + return renderCompletedView(m.editor.book.styles, m.editor.book.tmpls, "collection-add", viewData{ + Primary: m.editor.collection.Shelf.Name, + Secondary: m.editor.collection.Name, + }).Content } // Error returns the terminal error, if any, for the caller to surface after diff --git a/internal/model/mark_model.go b/internal/model/mark_model.go index 9d1df51..082f701 100644 --- a/internal/model/mark_model.go +++ b/internal/model/mark_model.go @@ -25,7 +25,7 @@ type markModel struct { func (m markModel) verifyCollection() bool { collection := m.book.form.GetString("collection") - validCollections := m.shelf.CollectionsNames() + validCollections := m.shelf.CollectionNames() return slices.Contains(validCollections, collection) } @@ -35,7 +35,7 @@ func (m markModel) verifyMark() bool { // Still needed for custom banner title if m.collection != nil { - validMarks := m.collection.MarksNames() + validMarks := m.collection.MarkNames() return slices.Contains(validMarks, mark) } return false @@ -199,7 +199,7 @@ func (m getMarkModel) View() tea.View { displayCollection = m.get.collection.Name if m.get.verifyMark() { - displayMark = m.get.mark.Name + "\n\n" + m.get.mark.URL + "\n\n" + lipglossList(s.None, m.get.mark.Tags) + "\n" + displayMark = m.get.mark.Title + "\n\n" + m.get.mark.URL + "\n\n" + lipglossList(s.None, m.get.mark.Tags) + "\n" } } } @@ -227,6 +227,24 @@ func (m getMarkModel) View() tea.View { return altScreenView(s.Base.Render(header + "\n" + body + "\n\n" + footer)) } +// markViewData builds the render data for a mark success screen. The parent +// section (the shelf and collection the mark belongs to) is rendered above the +// mark itself. +func markViewData(m *book.Mark) viewData { + data := viewData{Primary: m.Title, Secondary: m.URL, List: m.Tags} + if m.Shelf != nil || m.Collection != nil { + parent := &viewData{} + if m.Shelf != nil { + parent.Primary = m.Shelf.Name + } + if m.Collection != nil { + parent.Secondary = m.Collection.Name + } + data.Parent = parent + } + return data +} + // ResultView returns the completion output for the caller to print after the // program exits. func (m getMarkModel) ResultView() string { @@ -237,11 +255,15 @@ func (m getMarkModel) ResultView() string { t := m.get.book.tmpls switch m.action { case "get": - return renderCompletedView(s, t, "mark-get", m.get.mark).Content + return renderCompletedView(s, t, "mark-get", markViewData(m.get.mark)).Content case "list": - return renderCompletedView(s, t, "mark-list", m.get.collection).Content + return renderCompletedView(s, t, "mark-list", viewData{ + Primary: m.get.collection.Shelf.Name, + Secondary: m.get.collection.Name, + List: m.get.collection.MarkNames(), + }).Content case "delete": - return renderCompletedView(s, t, "mark-delete", m.get.mark).Content + return renderCompletedView(s, t, "mark-delete", markViewData(m.get.mark)).Content } return "" } @@ -311,7 +333,7 @@ func GetMarkForm(bs *book.BookShelves, mark *book.Mark, config *theme.UIConfig, if collection == nil { return []huh.Option[string]{} } - opts := collection.MarksNames() + opts := collection.MarkNames() return huh.NewOptions(opts...) }, &chosenCollection). Key("mark"). @@ -425,7 +447,7 @@ func (m editMarkModel) View() tea.View { currentShelf = s.StatusHeader.Render("Picked Shelf") + "\n" + shelf + "\n\n" currentCollection = s.StatusHeader.Render("Picked Collection") + "\n" + m.editor.mark.Collection.Name + "\n\n" - currentMark = s.StatusHeader.Render("Editing Mark") + "\n" + m.editor.mark.Name + currentMark = s.StatusHeader.Render("Editing Mark") + "\n" + m.editor.mark.Title currentMark += "\n\n" + m.editor.mark.URL + "\n\n" + lipglossList(s.None, m.editor.mark.Tags) + "\n" status = m.editor.book.statusPanel(form, currentShelf+currentCollection+currentMark, 28) @@ -455,9 +477,9 @@ func (m editMarkModel) ResultView() string { t := m.editor.book.tmpls switch m.action { case "add": - return renderCompletedView(s, t, "mark-add", m.editor.mark).Content + return renderCompletedView(s, t, "mark-add", markViewData(m.editor.mark)).Content case "edit": - return renderCompletedView(s, t, "mark-edit", m.editor.mark).Content + return renderCompletedView(s, t, "mark-edit", markViewData(m.editor.mark)).Content } return "" } @@ -485,7 +507,7 @@ func editMarkForm(bs *book.BookShelves, mark *book.Mark, config *theme.UIConfig, huh.NewText(). Title("Review title."). Key("markTitle"). - Value(&m.mark.Name). + Value(&m.mark.Title). WithWidth(25). WithHeight(3), diff --git a/internal/model/shelf_model.go b/internal/model/shelf_model.go index 105c027..3c4ba8c 100644 --- a/internal/model/shelf_model.go +++ b/internal/model/shelf_model.go @@ -111,7 +111,9 @@ func (m getShelfModel) ResultView() string { if m.get.book.form.State != huh.StateCompleted || m.get.book.err != nil || m.action != "list" { return "" } - return renderCompletedView(m.get.book.styles, m.get.book.tmpls, "shelf-list", m.get.book.shelves).Content + return renderCompletedView(m.get.book.styles, m.get.book.tmpls, "shelf-list", viewData{ + List: m.get.book.shelves.ShelfNames(), + }).Content } // Error returns the terminal error, if any, for the caller to surface after @@ -296,7 +298,10 @@ func (m editShelfModel) ResultView() string { if m.editor.book.form.State != huh.StateCompleted || m.editor.book.err != nil { return "" } - return renderCompletedView(m.editor.book.styles, m.editor.book.tmpls, "shelf-add", m.editor.collection).Content + return renderCompletedView(m.editor.book.styles, m.editor.book.tmpls, "shelf-add", viewData{ + Primary: m.editor.collection.Shelf.Name, + Secondary: m.editor.collection.Name, + }).Content } // Error returns the terminal error, if any, for the caller to surface after diff --git a/internal/model/tea.go b/internal/model/tea.go index 730199f..4a25dd1 100644 --- a/internal/model/tea.go +++ b/internal/model/tea.go @@ -225,37 +225,51 @@ func lipglossList(gloss lipgloss.Style, l []string) string { return strings.Join(parts, "\n") } -func renderCompletedView(s *Styles, tmpls map[string]book.ViewTemplate, key string, entity book.Templatable) tea.View { +// viewData is the presentation data a completed TUI screen renders. Each screen +// builds it from the domain entities it already holds, so the renderer stays a +// dumb formatter and the meaning of each field is decided per screen rather +// than by a shared interface on the domain types. +type viewData struct { + Primary string + Secondary string + List []string + // Parent, when set, is rendered above the main section with the + // "mark-parent" template (the shelf and collection a mark belongs to) and + // is followed by a blank line separating it from the main section. + Parent *viewData +} + +func renderCompletedView(s *Styles, tmpls map[string]book.ViewTemplate, key string, data viewData) tea.View { tmpl := tmpls[key] var b strings.Builder - if strings.HasPrefix(key, "mark-") { - if mark, ok := entity.(*book.Mark); ok { - fmt.Fprintf(&b, "%s", renderView(s, tmpls["mark-parent"], mark.Collection)) + if data.Parent != nil { + if parent := renderView(s, tmpls["mark-parent"], *data.Parent); parent != "" { + fmt.Fprintf(&b, "%s\n\n", parent) } } - fmt.Fprintf(&b, "%s", renderView(s, tmpl, entity)) + fmt.Fprintf(&b, "%s", renderView(s, tmpl, data)) return tea.NewView(s.StatusBox.Render(b.String()) + "\n") } -func renderView(styles *Styles, tmpl book.ViewTemplate, entity book.Templatable) string { +func renderView(styles *Styles, tmpl book.ViewTemplate, data viewData) string { var b strings.Builder - if tmpl.PrimaryTitle != "" && entity.Primary() != "" { - fmt.Fprintf(&b, "%s\n%s\n\n", tmpl.PrimaryTitle, styles.Primary.Render(entity.Primary())) + if tmpl.PrimaryTitle != "" && data.Primary != "" { + fmt.Fprintf(&b, "%s\n%s\n\n", tmpl.PrimaryTitle, styles.Primary.Render(data.Primary)) } - if tmpl.SecondaryTitle != "" && entity.Secondary() != "" { - fmt.Fprintf(&b, "%s\n%s", tmpl.SecondaryTitle, styles.Primary.Render(entity.Secondary())) + if tmpl.SecondaryTitle != "" && data.Secondary != "" { + fmt.Fprintf(&b, "%s\n%s", tmpl.SecondaryTitle, styles.Primary.Render(data.Secondary)) } // needed for mark rendering - if entity.Secondary() != "" && len(entity.List()) > 0 { + if data.Secondary != "" && len(data.List) > 0 { fmt.Fprintf(&b, "\n\n") } - if tmpl.ListTitle != "" && len(entity.List()) > 0 { - fmt.Fprintf(&b, "%s\n%s", tmpl.ListTitle, lipglossList(styles.Primary, entity.List())) + if tmpl.ListTitle != "" && len(data.List) > 0 { + fmt.Fprintf(&b, "%s\n%s", tmpl.ListTitle, lipglossList(styles.Primary, data.List)) } return b.String() diff --git a/internal/model/tea_test.go b/internal/model/tea_test.go new file mode 100644 index 0000000..028a739 --- /dev/null +++ b/internal/model/tea_test.go @@ -0,0 +1,87 @@ +package model + +import ( + "strings" + "testing" + + "charm.land/lipgloss/v2" + + "github.com/polymorcodeus/book/pkg/book" +) + +// plainStyles returns Styles with no color or spacing so rendered output is a +// stable string in tests. +func plainStyles() *Styles { + return &Styles{ + Primary: lipgloss.NewStyle(), + StatusBox: lipgloss.NewStyle(), + None: lipgloss.NewStyle(), + } +} + +func TestRenderViewSections(t *testing.T) { + tmpl := book.ViewTemplate{ + PrimaryTitle: "To add mark:", + SecondaryTitle: "With URL:", + ListTitle: "With tags:", + } + got := renderView(plainStyles(), tmpl, viewData{ + Primary: "The Go Programming Language", + Secondary: "https://go.dev", + List: []string{"lang", "official"}, + }) + + want := "To add mark:\nThe Go Programming Language\n\n" + + "With URL:\nhttps://go.dev\n\n" + + "With tags:\n" + listBullet + " lang\n" + listBullet + " official" + if got != want { + t.Errorf("renderView() = %q, want %q", got, want) + } +} + +func TestRenderViewSkipsEmptySections(t *testing.T) { + tmpl := book.ViewTemplate{ListTitle: "Shelves:"} + got := renderView(plainStyles(), tmpl, viewData{List: []string{"archive"}}) + + if strings.Contains(got, "To add") { + t.Errorf("renderView() rendered an unset section: %q", got) + } + if want := "Shelves:\n" + listBullet + " archive"; got != want { + t.Errorf("renderView() = %q, want %q", got, want) + } +} + +func TestRenderCompletedViewRendersParentFirst(t *testing.T) { + got := renderCompletedView(plainStyles(), book.DefaultViewTemplates, "mark-add", viewData{ + Primary: "The Go Programming Language", + Secondary: "https://go.dev", + List: []string{"lang"}, + Parent: &viewData{Primary: "work", Secondary: "golang"}, + }).Content + + shelf := strings.Index(got, "Chosen shelf:\nwork") + collection := strings.Index(got, "Chosen collection:\ngolang") + mark := strings.Index(got, "To add mark:\nThe Go Programming Language") + if shelf < 0 || collection < 0 || mark < 0 { + t.Fatalf("renderCompletedView() missing a section: %q", got) + } + if shelf >= collection || collection >= mark { + t.Errorf("parent section not rendered above the mark: %q", got) + } + if !strings.Contains(got, "golang\n\nTo add mark:") { + t.Errorf("parent and mark sections are not separated by a blank line: %q", got) + } +} + +func TestRenderCompletedViewOmitsParentWhenUnset(t *testing.T) { + got := renderCompletedView(plainStyles(), book.DefaultViewTemplates, "shelf-list", viewData{ + List: []string{"archive", "dev"}, + }).Content + + if strings.Contains(got, "Chosen collection:") { + t.Errorf("renderCompletedView() rendered a parent section with no Parent set: %q", got) + } + if !strings.Contains(got, "You've knocked over all your shelves:") { + t.Errorf("renderCompletedView() missing the list title: %q", got) + } +} diff --git a/pkg/book/doctor.go b/pkg/book/doctor.go index d952710..70e86ff 100644 --- a/pkg/book/doctor.go +++ b/pkg/book/doctor.go @@ -35,7 +35,7 @@ func (bs *BookShelves) DetectDuplicates() []MarkConflict { for i := range *bs { shelf := &(*bs)[i] - for _, name := range shelf.CollectionsNames() { + for _, name := range shelf.CollectionNames() { c := shelf.Collections[name] for _, m := range c.Marks { id := effectiveID(m) @@ -107,7 +107,7 @@ func (bs *BookShelves) ResolveDuplicates() (removed int, changed []*Shelf) { func marksIdentical(marks []*Mark) bool { first := marks[0] for _, m := range marks[1:] { - if m.Name != first.Name || m.URL != first.URL || m.DeletedAt != first.DeletedAt { + if m.Title != first.Title || m.URL != first.URL || m.DeletedAt != first.DeletedAt { return false } if !equalTags(m.Tags, first.Tags) { diff --git a/pkg/book/doctor_test.go b/pkg/book/doctor_test.go index ff2b532..a508641 100644 --- a/pkg/book/doctor_test.go +++ b/pkg/book/doctor_test.go @@ -5,12 +5,12 @@ import ( ) // mark returns a Mark with back-pointers wired to the given shelf and collection. -func testMark(s *Shelf, c *Collection, id, name, url string, tags []string) *Mark { +func testMark(s *Shelf, c *Collection, id, title, url string, tags []string) *Mark { return &Mark{ Shelf: s, Collection: c, ID: id, - Name: name, + Title: title, URL: url, Tags: tags, } diff --git a/pkg/book/example_test.go b/pkg/book/example_test.go index dbc73c9..17166a1 100644 --- a/pkg/book/example_test.go +++ b/pkg/book/example_test.go @@ -32,7 +32,7 @@ func Example() { if err != nil { panic(err) } - mark.Name = "The Go Programming Language" + mark.Title = "The Go Programming Language" mark.Shelf = shelf mark.Collection = collection mark.RecordAdd() @@ -55,7 +55,7 @@ func Example() { } got := loaded.Collection("golang").Mark("The Go Programming Language") fmt.Println(loaded.Name) - fmt.Println(got.Name) + fmt.Println(got.Title) fmt.Println(got.URL) fmt.Println(got.Tags) diff --git a/pkg/book/templates.go b/pkg/book/templates.go index 908284c..ed205db 100644 --- a/pkg/book/templates.go +++ b/pkg/book/templates.go @@ -1,48 +1,5 @@ package book -// Templatable defines the view-model interface used to render TUI success screens. -type Templatable interface { - Primary() string - Secondary() string - List() []string -} - -// Primary returns an empty string for BookShelves. -func (bs *BookShelves) Primary() string { return "" } - -// Secondary returns an empty string for BookShelves. -func (bs *BookShelves) Secondary() string { return "" } - -// List returns the names of all shelves. -func (bs *BookShelves) List() []string { return bs.ShelfNames() } - -// Primary returns the shelf name. -func (s *Shelf) Primary() string { return s.Name } - -// Secondary returns an empty string for Shelf. -func (s *Shelf) Secondary() string { return "" } - -// List returns the names of all collections in the shelf. -func (s *Shelf) List() []string { return s.CollectionsNames() } - -// Primary returns the parent shelf name. -func (c *Collection) Primary() string { return c.Shelf.Name } - -// Secondary returns the collection name. -func (c *Collection) Secondary() string { return c.Name } - -// List returns the names of all marks in the collection. -func (c *Collection) List() []string { return c.MarksNames() } - -// Primary returns the mark title. -func (m *Mark) Primary() string { return m.Name } - -// Secondary returns the mark URL. -func (m *Mark) Secondary() string { return m.URL } - -// List returns the mark's tags. -func (m *Mark) List() []string { return m.Tags } - // ViewTemplate used to templatize TUI success screens type ViewTemplate struct { PrimaryTitle string `json:"primary_title,omitempty"` diff --git a/pkg/book/types.go b/pkg/book/types.go index 80b7904..dd1707f 100644 --- a/pkg/book/types.go +++ b/pkg/book/types.go @@ -9,6 +9,7 @@ import ( "bytes" "crypto/sha256" "encoding/json" + "errors" "fmt" "net/url" "slices" @@ -18,6 +19,19 @@ import ( "github.com/BurntSushi/toml" ) +// Sentinel errors returned by the pure domain helpers. Callers match them with +// errors.Is and render their own remediation text; the domain package never +// references CLI flags or command names. +var ( + // ErrDuplicateURL reports that a mark with the same catalog ID already exists. + ErrDuplicateURL = errors.New("book: duplicate url") + // ErrURLTrashed reports that a mark with the same catalog ID was soft-deleted. + ErrURLTrashed = errors.New("book: url already trashed") + // ErrTitleRequired reports that a title could not be resolved automatically + // and the caller must supply one. + ErrTitleRequired = errors.New("book: title required") +) + // TOMLFile defines exportable TOML files type TOMLFile interface { FileDetail() string @@ -27,21 +41,21 @@ type TOMLFile interface { type Config struct { CatalogFormat string `toml:"catalog_format"` ShelfRoot string `toml:"shelf_directory"` - Autoconfirm bool `toml:"autoconfirm"` // edit to bypass --confirm for non-interactive adds - Interactive bool `toml:"interactive"` // edit to bypass TUI - false by default + Autoconfirm bool `toml:"autoconfirm"` // bypass --confirm for non-interactive writes + Interactive bool `toml:"interactive"` // true enables the TUI; false by default ConfigFile string `toml:"-"` // path to config file, typical BOOK_CONFIG ThemeFile string `toml:"theme_file"` // path to theme file, typical BOOK_THEME - TemplateFile string `toml:"template_file"` // path to theme file, typical BOOK_TEMPLATE + TemplateFile string `toml:"template_file"` // path to template file, typical BOOK_TEMPLATE } // FileConfig holds externally writable application configuration settings type FileConfig struct { CatalogFormat string `toml:"catalog_format"` ShelfRoot string `toml:"shelf_directory"` - Autoconfirm *bool `toml:"autoconfirm"` // edit to bypass --confirm for non-interactive adds - Interactive *bool `toml:"interactive"` // edit to bypass TUI - false by default + Autoconfirm *bool `toml:"autoconfirm"` // bypass --confirm for non-interactive writes + Interactive *bool `toml:"interactive"` // true enables the TUI; false by default ThemeFile string `toml:"theme_file"` // path to theme file, typical BOOK_THEME - TemplateFile string `toml:"template_file"` // path to theme file, typical BOOK_TEMPLATE + TemplateFile string `toml:"template_file"` // path to template file, typical BOOK_TEMPLATE ConfigFile string `toml:"-"` // path to config file, typical BOOK_CONFIG } @@ -96,8 +110,9 @@ func (bs *BookShelves) LoadParents() { } // VerifyUniqueURL returns an error if the given ID already exists in any mark -// other than exclude. A collision with a soft-deleted mark points at -// `book mark restore` rather than re-adding the URL. +// other than exclude. A collision with a soft-deleted mark wraps +// ErrURLTrashed; an active collision wraps ErrDuplicateURL. Callers render any +// remediation text (for example how to restore a trashed mark). func (bs *BookShelves) VerifyUniqueURL(id string, exclude *Mark) error { for _, b := range *bs { for _, c := range b.Collections { @@ -106,10 +121,9 @@ func (bs *BookShelves) VerifyUniqueURL(id string, exclude *Mark) error { continue } if m.IsDeleted() { - return fmt.Errorf("url already trashed\n\n%s\n\nrestore it with:\nbook mark restore --shelf %s --collection %s --url %s", - m.FullDetail(), m.Shelf.Name, m.Collection.Name, m.URL) + return fmt.Errorf("%w: %s", ErrURLTrashed, m.FullDetail()) } - return fmt.Errorf("duplicate url found\n\n%s", m.FullDetail()) + return fmt.Errorf("%w: %s", ErrDuplicateURL, m.FullDetail()) } } } @@ -127,14 +141,17 @@ func (bs *BookShelves) PurgeDeletedMarks(cutoff time.Time) int { } // SoftDeletedByID returns the soft-deleted mark whose ID matches, or nil. IDs -// are globally unique, so no shelf or collection scoping is needed. The -// returned mark retains its Shelf and Collection back-pointers after -// LoadParents. +// are globally unique, so no shelf or collection scoping is needed. Like the +// other finders, the returned mark has its Shelf and Collection back-pointers +// wired. func (bs *BookShelves) SoftDeletedByID(id string) *Mark { for i := range *bs { - for _, c := range (*bs)[i].Collections { + shelf := &(*bs)[i] + for _, c := range shelf.Collections { for _, m := range c.Marks { if m.ID == id && m.IsDeleted() { + m.Shelf = shelf + m.Collection = c return m } } @@ -195,8 +212,8 @@ func (s *Shelf) PurgeDeletedMarks(cutoff time.Time) int { return total } -// CollectionsNames returns the names of all collections in the shelf, sorted. -func (s *Shelf) CollectionsNames() []string { +// CollectionNames returns the names of all collections in the shelf, sorted. +func (s *Shelf) CollectionNames() []string { names := make([]string, 0, len(s.Collections)) for name := range s.Collections { names = append(names, name) @@ -216,14 +233,14 @@ type Collection struct { Marks []*Mark `toml:"marks" json:"marks"` } -// MarksNames returns the names of all non-deleted marks in the collection. -func (c *Collection) MarksNames() []string { +// MarkNames returns the titles of all non-deleted marks in the collection. +func (c *Collection) MarkNames() []string { markNames := make([]string, 0, len(c.Marks)) for _, m := range c.Marks { if m.IsDeleted() { continue } - markNames = append(markNames, m.Name) + markNames = append(markNames, m.Title) } return markNames } @@ -251,10 +268,10 @@ func (c *Collection) AllTags() []string { return tags } -// Mark returns a non-deleted mark by name from the collection. +// Mark returns a non-deleted mark by title from the collection. func (c *Collection) Mark(m string) *Mark { for _, n := range c.Marks { - if n.Name == m && !n.IsDeleted() { + if n.Title == m && !n.IsDeleted() { return n } } @@ -320,7 +337,7 @@ type Mark struct { Collection *Collection `toml:"-" json:"-"` ID string `toml:"catalog_id" json:"catalog_id"` - Name string `toml:"title" json:"title"` + Title string `toml:"title" json:"title"` URL string `toml:"url" json:"url"` Tags []string `toml:"tags" json:"tags"` CreatedAt string `toml:"created_at,omitempty" json:"created_at,omitempty"` @@ -330,7 +347,7 @@ type Mark struct { // Description returns a human-readable summary of the mark. func (m *Mark) Description() string { - return fmt.Sprintf("Title: %s\nURL: %s\nTags: %s", m.Name, m.URL, strings.Join(m.Tags, ",")) + return fmt.Sprintf("Title: %s\nURL: %s\nTags: %s", m.Title, m.URL, strings.Join(m.Tags, ",")) } // IsDeleted reports whether the mark has been soft-deleted. @@ -367,9 +384,18 @@ func (m *Mark) RecordDelete() { m.Touch() } -// FullDetail returns a verbose summary including shelf, collection, title, URL, and tags. +// FullDetail returns a verbose summary including shelf, collection, title, URL, +// and tags. Missing back-pointers (an unwired mark) render as empty fields +// rather than panicking. func (m *Mark) FullDetail() string { - return fmt.Sprintf("Shelf: %s\nCollection: %s\nTitle: %s\nURL: %s\nTags: %s", m.Shelf.Name, m.Collection.Name, m.Name, m.URL, strings.Join(m.Tags, ",")) + shelfName, collectionName := "", "" + if m.Shelf != nil { + shelfName = m.Shelf.Name + } + if m.Collection != nil { + collectionName = m.Collection.Name + } + return fmt.Sprintf("Shelf: %s\nCollection: %s\nTitle: %s\nURL: %s\nTags: %s", shelfName, collectionName, m.Title, m.URL, strings.Join(m.Tags, ",")) } // dedupUnique concatenates and deduplicates multiple slices while preserving first-seen order. @@ -410,8 +436,8 @@ func NowTimestamp() string { } // MergeTags combines multiple tag slices, deduplicates, removes empty strings, -// and returns a sorted slice. Order of arguments determines priority (earlier -// slices' items appear first in result). +// and preserves first-seen order: earlier slices' items appear first and no +// sorting is performed. func MergeTags(sources ...[]string) []string { merged := dedupUnique(sources...) return slices.DeleteFunc(merged, func(e string) bool { return e == "" }) @@ -468,7 +494,7 @@ type TitleFetchResult struct { // ResolveMarkTitle selects the title for a new mark. A caller-provided title // always wins. When fetching is unavailable, interactive callers receive an // empty string (so the TUI can prompt later), while non-interactive callers -// receive an error asking them to provide --title. +// receive an error wrapping ErrTitleRequired. func ResolveMarkTitle(providedTitle, url string, fetched TitleFetchResult, interactive bool) (string, error) { if providedTitle != "" { return providedTitle, nil @@ -477,16 +503,46 @@ func ResolveMarkTitle(providedTitle, url string, fetched TitleFetchResult, inter if interactive { return "", nil } - return "", fmt.Errorf("couldn't fetch title for %s; provide --title", url) + return "", fmt.Errorf("%w for %s", ErrTitleRequired, url) } return fetched.Title, nil } -// ValidateNewShelfName returns an error if name is empty or already in use. -func (bs *BookShelves) ValidateNewShelfName(name string) error { - if name == "" { +// invalidShelfNameChars are characters that cannot appear in a shelf name: the +// path separators (which would make the name produce a nested file path) plus +// the set that is invalid in a Windows file name. +const invalidShelfNameChars = `/\:*?"<>|` + +// validateShelfName returns an error when name cannot safely be used as a shelf +// name. The name must contain visible characters with no leading or trailing +// whitespace (ShelfPath trims and underscorifies names, so padded names would +// collide with their trimmed form on disk), must not be a parent-directory +// token, and must not contain path separators, control characters, or +// characters reserved by the file system. +func validateShelfName(name string) error { + if strings.TrimSpace(name) == "" { return fmt.Errorf("shelf name is required") } + if name != strings.TrimSpace(name) { + return fmt.Errorf("shelf name %q has leading or trailing whitespace", name) + } + if name == "." || name == ".." { + return fmt.Errorf("shelf name %q is not allowed", name) + } + for _, r := range name { + if r < 0x20 || r == 0x7f || strings.ContainsRune(invalidShelfNameChars, r) { + return fmt.Errorf("shelf name %q contains invalid characters", name) + } + } + return nil +} + +// ValidateNewShelfName returns an error if name is empty, contains invalid +// characters, or is already in use. +func (bs *BookShelves) ValidateNewShelfName(name string) error { + if err := validateShelfName(name); err != nil { + return err + } if slices.Contains(bs.ShelfNames(), name) { return fmt.Errorf("shelf already exists") } @@ -531,7 +587,8 @@ func ParseTagFilter(input string) ([][]string, error) { return result, nil } -// MarshalCatalog serializes an item as JSON or TOML. +// MarshalCatalog serializes an item as JSON or TOML. An unknown format is an +// error. func MarshalCatalog[T any](item T, format string) ([]byte, error) { switch format { case "json": @@ -543,13 +600,14 @@ func MarshalCatalog[T any](item T, format string) ([]byte, error) { } return buf.Bytes(), nil } - return nil, nil + return nil, fmt.Errorf("unknown format %q", format) } -// NewShelf creates a new v2 shelf with the given name and description. +// NewShelf creates a new v2 shelf with the given name and description. The name +// is validated for emptiness and unsafe characters. func NewShelf(name, description string) (*Shelf, error) { - if name == "" { - return nil, fmt.Errorf("shelf name is required") + if err := validateShelfName(name); err != nil { + return nil, err } now := NowTimestamp() return &Shelf{ @@ -605,7 +663,8 @@ func (s *Shelf) RemoveCollection(name string) error { } // FindMarkByID returns the first mark matching id across all shelves and -// collections, or nil if none is found. +// collections, or nil if none is found. The returned mark has its Shelf and +// Collection back-pointers wired. func (bs *BookShelves) FindMarkByID(id string) *Mark { for i := range *bs { shelf := &(*bs)[i] @@ -623,7 +682,8 @@ func (bs *BookShelves) FindMarkByID(id string) *Mark { } // FindMarkByURL returns the first non-deleted mark matching url across all -// shelves and collections, or nil if none is found. +// shelves and collections, or nil if none is found. The returned mark has its +// Shelf and Collection back-pointers wired. func (bs *BookShelves) FindMarkByURL(url string) *Mark { for i := range *bs { shelf := &(*bs)[i] @@ -640,11 +700,11 @@ func (bs *BookShelves) FindMarkByURL(url string) *Mark { return nil } -// UpdateMark applies edits to a mark. Empty strings leave title and url +// Update applies edits to a mark. Empty strings leave title and url // unchanged; a nil tags slice leaves tags unchanged, while an empty (non-nil) // slice clears them. If url is provided it is validated and the mark's catalog // ID is regenerated. -func (m *Mark) UpdateMark(title, url string, tags []string) error { +func (m *Mark) Update(title, url string, tags []string) error { if url != "" { if err := ValidateURL(url); err != nil { return err @@ -653,7 +713,7 @@ func (m *Mark) UpdateMark(title, url string, tags []string) error { m.ID = GenerateID(url) } if title != "" { - m.Name = title + m.Title = title } if tags != nil { m.Tags = tags diff --git a/pkg/book/types_test.go b/pkg/book/types_test.go index 4d353e7..8838936 100644 --- a/pkg/book/types_test.go +++ b/pkg/book/types_test.go @@ -1,6 +1,7 @@ package book import ( + "errors" "reflect" "slices" "strings" @@ -68,8 +69,8 @@ func TestVerifyUniqueURL(t *testing.T) { "col-1": { Name: "col-1", Marks: []*Mark{ - {ID: "abc12345", Name: "first", URL: "https://example.com/first"}, - {ID: "aabbccdd", Name: "trashed", URL: "https://example.com/trashed", DeletedAt: "2026-08-01T00:00:00Z"}, + {ID: "abc12345", Title: "first", URL: "https://example.com/first"}, + {ID: "aabbccdd", Title: "trashed", URL: "https://example.com/trashed", DeletedAt: "2026-08-01T00:00:00Z"}, }, }, }, @@ -80,7 +81,7 @@ func TestVerifyUniqueURL(t *testing.T) { "col-2": { Name: "col-2", Marks: []*Mark{ - {ID: "def67890", Name: "second", URL: "https://example.com/second"}, + {ID: "def67890", Title: "second", URL: "https://example.com/second"}, }, }, }, @@ -89,10 +90,10 @@ func TestVerifyUniqueURL(t *testing.T) { bs.LoadParents() tests := []struct { - name string - id string - wantErr bool - wantRestore bool + name string + id string + wantErr bool + wantSentinel error }{ { name: "unique id passes", @@ -100,20 +101,22 @@ func TestVerifyUniqueURL(t *testing.T) { wantErr: false, }, { - name: "duplicate in first shelf", - id: "abc12345", - wantErr: true, + name: "duplicate in first shelf", + id: "abc12345", + wantErr: true, + wantSentinel: ErrDuplicateURL, }, { - name: "duplicate in second shelf", - id: "def67890", - wantErr: true, + name: "duplicate in second shelf", + id: "def67890", + wantErr: true, + wantSentinel: ErrDuplicateURL, }, { - name: "trashed collision suggests restore", - id: "aabbccdd", - wantErr: true, - wantRestore: true, + name: "trashed collision", + id: "aabbccdd", + wantErr: true, + wantSentinel: ErrURLTrashed, }, } @@ -126,8 +129,8 @@ func TestVerifyUniqueURL(t *testing.T) { if !tt.wantErr && err != nil { t.Errorf("VerifyUniqueURL(%q) unexpected error: %v", tt.id, err) } - if tt.wantRestore && (err == nil || !strings.Contains(err.Error(), "restore")) { - t.Errorf("VerifyUniqueURL(%q) error = %v, want restore hint", tt.id, err) + if tt.wantSentinel != nil && !errors.Is(err, tt.wantSentinel) { + t.Errorf("VerifyUniqueURL(%q) error = %v, want %v", tt.id, err, tt.wantSentinel) } }) } @@ -167,8 +170,8 @@ func TestAllTags(t *testing.T) { } func TestDeleteMark(t *testing.T) { - markA := &Mark{Name: "a"} - markB := &Mark{Name: "b"} + markA := &Mark{Title: "a"} + markB := &Mark{Title: "b"} col := &Collection{Marks: []*Mark{markA, markB}} col.DeleteMark(markB) @@ -209,23 +212,23 @@ func TestIsDeleted(t *testing.T) { } } -func TestMarksNamesExcludesDeleted(t *testing.T) { +func TestMarkNamesExcludesDeleted(t *testing.T) { col := &Collection{Marks: []*Mark{ - {Name: "a"}, - {Name: "b", DeletedAt: "x"}, - {Name: "c"}, + {Title: "a"}, + {Title: "b", DeletedAt: "x"}, + {Title: "c"}, }} want := []string{"a", "c"} - got := col.MarksNames() + got := col.MarkNames() if !slices.Equal(got, want) { - t.Errorf("MarksNames() = %v, want %v", got, want) + t.Errorf("MarkNames() = %v, want %v", got, want) } } func TestMarkSkipsDeleted(t *testing.T) { col := &Collection{Marks: []*Mark{ - {Name: "a", DeletedAt: "x"}, - {Name: "a"}, + {Title: "a", DeletedAt: "x"}, + {Title: "a"}, }} got := col.Mark("a") if got == nil || got.DeletedAt != "" { @@ -262,17 +265,17 @@ func TestPurgeDeletedMarks(t *testing.T) { { name: "purges only old soft-deleted marks", col: &Collection{Marks: []*Mark{ - {Name: "active"}, - {Name: "old", DeletedAt: old}, - {Name: "recent", DeletedAt: recent}, - {Name: "bad", DeletedAt: "not-a-timestamp"}, + {Title: "active"}, + {Title: "old", DeletedAt: old}, + {Title: "recent", DeletedAt: recent}, + {Title: "bad", DeletedAt: "not-a-timestamp"}, }}, want: 1, }, { name: "no soft-deleted marks", col: &Collection{Marks: []*Mark{ - {Name: "active"}, + {Title: "active"}, }}, want: 0, }, @@ -289,15 +292,15 @@ func TestPurgeDeletedMarks(t *testing.T) { // Verify the first case retains the right marks. col := &Collection{Marks: []*Mark{ - {Name: "active"}, - {Name: "old", DeletedAt: old}, - {Name: "recent", DeletedAt: recent}, - {Name: "bad", DeletedAt: "not-a-timestamp"}, + {Title: "active"}, + {Title: "old", DeletedAt: old}, + {Title: "recent", DeletedAt: recent}, + {Title: "bad", DeletedAt: "not-a-timestamp"}, }} col.PurgeDeletedMarks(cutoff) var remaining []string for _, m := range col.Marks { - remaining = append(remaining, m.Name) + remaining = append(remaining, m.Title) } want := []string{"active", "recent", "bad"} if !slices.Equal(remaining, want) { @@ -313,15 +316,15 @@ func TestSoftDeletedByID(t *testing.T) { "col-1": { Name: "col-1", Marks: []*Mark{ - {ID: "abc12345", Name: "active", URL: "https://example.com"}, - {ID: "aabbccdd", Name: "trashed", URL: "https://example.com/trashed", DeletedAt: "2026-08-01T00:00:00Z"}, + {ID: "abc12345", Title: "active", URL: "https://example.com"}, + {ID: "aabbccdd", Title: "trashed", URL: "https://example.com/trashed", DeletedAt: "2026-08-01T00:00:00Z"}, }, }, }, }, } - if got := bs.SoftDeletedByID("aabbccdd"); got == nil || got.Name != "trashed" { + if got := bs.SoftDeletedByID("aabbccdd"); got == nil || got.Title != "trashed" { t.Errorf("SoftDeletedByID(trashed) = %+v, want trashed mark", got) } if got := bs.SoftDeletedByID("abc12345"); got != nil { @@ -618,15 +621,46 @@ func TestValidateNewShelfName(t *testing.T) { if err := bs.ValidateNewShelfName(""); err == nil { t.Error("ValidateNewShelfName(\"\") expected error") } + if err := bs.ValidateNewShelfName(" "); err == nil { + t.Error("ValidateNewShelfName(whitespace) expected error") + } if err := bs.ValidateNewShelfName("existing"); err == nil { t.Error("ValidateNewShelfName(\"existing\") expected error") } + + invalid := []string{ + "../escape", + "nested/name", + `back\slash`, + ".", + "..", + "colon:name", + "star*name", + "line\nbreak", + " padded", + "padded ", + "padded\n", + } + for _, name := range invalid { + if err := bs.ValidateNewShelfName(name); err == nil { + t.Errorf("ValidateNewShelfName(%q) expected error", name) + } + } } func TestNewShelf(t *testing.T) { if _, err := NewShelf("", ""); err == nil { t.Error("NewShelf with empty name expected error") } + if _, err := NewShelf("../escape", ""); err == nil { + t.Error("NewShelf with path separator expected error") + } + if _, err := NewShelf("..", ""); err == nil { + t.Error("NewShelf with parent token expected error") + } + if _, err := NewShelf(" padded", ""); err == nil { + t.Error("NewShelf with leading whitespace expected error") + } s, err := NewShelf("test-shelf", "a description") if err != nil { @@ -726,7 +760,7 @@ func TestBookShelvesFindMarkByID(t *testing.T) { "col-1": { Name: "col-1", Marks: []*Mark{ - {ID: "abc12345", Name: "first", URL: "https://example.com/first"}, + {ID: "abc12345", Title: "first", URL: "https://example.com/first"}, }, }, }, @@ -737,8 +771,8 @@ func TestBookShelvesFindMarkByID(t *testing.T) { if got == nil { t.Fatal("FindMarkByID expected match") } - if got.Name != "first" { - t.Errorf("FindMarkByID Name = %q, want %q", got.Name, "first") + if got.Title != "first" { + t.Errorf("FindMarkByID Title = %q, want %q", got.Title, "first") } if got.Shelf == nil || got.Shelf.Name != "shelf-a" { t.Error("FindMarkByID did not set Shelf back-pointer") @@ -752,19 +786,19 @@ func TestBookShelvesFindMarkByID(t *testing.T) { } } -func TestMarkUpdateMark(t *testing.T) { +func TestMarkUpdate(t *testing.T) { m := &Mark{ - ID: GenerateID("https://example.com/old"), - Name: "Old", - URL: "https://example.com/old", - Tags: []string{"a"}, + ID: GenerateID("https://example.com/old"), + Title: "Old", + URL: "https://example.com/old", + Tags: []string{"a"}, } - if err := m.UpdateMark("New", "", []string{"b", "c"}); err != nil { + if err := m.Update("New", "", []string{"b", "c"}); err != nil { t.Fatalf("UpdateMark error: %v", err) } - if m.Name != "New" { - t.Errorf("Name = %q, want %q", m.Name, "New") + if m.Title != "New" { + t.Errorf("Title = %q, want %q", m.Title, "New") } if !slices.Equal(m.Tags, []string{"b", "c"}) { t.Errorf("Tags = %v, want %v", m.Tags, []string{"b", "c"}) @@ -773,11 +807,11 @@ func TestMarkUpdateMark(t *testing.T) { t.Errorf("URL = %q, want unchanged", m.URL) } - if err := m.UpdateMark("", "not-a-url", nil); err == nil { + if err := m.Update("", "not-a-url", nil); err == nil { t.Error("UpdateMark with invalid URL expected error") } - if err := m.UpdateMark("", "https://example.com/new", nil); err != nil { + if err := m.Update("", "https://example.com/new", nil); err != nil { t.Fatalf("UpdateMark URL change error: %v", err) } if m.URL != "https://example.com/new" { @@ -796,8 +830,8 @@ func TestBookShelvesFindMarkByURL(t *testing.T) { "col-1": { Name: "col-1", Marks: []*Mark{ - {ID: "abc12345", Name: "first", URL: "https://example.com/first"}, - {ID: "deadbeef", Name: "trashed", URL: "https://example.com/trashed", DeletedAt: "2026-08-01T00:00:00Z"}, + {ID: "abc12345", Title: "first", URL: "https://example.com/first"}, + {ID: "deadbeef", Title: "trashed", URL: "https://example.com/trashed", DeletedAt: "2026-08-01T00:00:00Z"}, }, }, }, @@ -809,8 +843,8 @@ func TestBookShelvesFindMarkByURL(t *testing.T) { if got == nil { t.Fatal("FindMarkByURL expected match") } - if got.Name != "first" { - t.Errorf("Name = %q, want %q", got.Name, "first") + if got.Title != "first" { + t.Errorf("Name = %q, want %q", got.Title, "first") } if got.Shelf == nil || got.Shelf.Name != "shelf-a" { t.Error("FindMarkByURL did not set Shelf back-pointer") @@ -827,7 +861,7 @@ func TestBookShelvesFindMarkByURL(t *testing.T) { func TestMarkTouch(t *testing.T) { shelf := &Shelf{Name: "shelf-a", SchemaVersion: new(2), UpdatedAt: "old"} collection := &Collection{Name: "col-1", Shelf: shelf, UpdatedAt: "old"} - mark := &Mark{Name: "mark", Shelf: shelf, Collection: collection} + mark := &Mark{Title: "mark", Shelf: shelf, Collection: collection} mark.Touch() @@ -845,7 +879,7 @@ func TestMarkTouch(t *testing.T) { func TestMarkRecordAdd(t *testing.T) { shelf := &Shelf{Name: "shelf-a", SchemaVersion: new(2)} collection := &Collection{Name: "col-1", Shelf: shelf} - mark := &Mark{Name: "mark", Shelf: shelf, Collection: collection} + mark := &Mark{Title: "mark", Shelf: shelf, Collection: collection} mark.RecordAdd() @@ -863,7 +897,7 @@ func TestMarkRecordAdd(t *testing.T) { func TestMarkRecordDelete(t *testing.T) { shelf := &Shelf{Name: "shelf-a", SchemaVersion: new(2)} collection := &Collection{Name: "col-1", Shelf: shelf, Marks: make([]*Mark, 0)} - mark := &Mark{Name: "mark", Shelf: shelf, Collection: collection} + mark := &Mark{Title: "mark", Shelf: shelf, Collection: collection} collection.AddMark(mark) mark.RecordDelete() @@ -881,21 +915,21 @@ func TestMarkRecordDelete(t *testing.T) { func TestUpdateMarkClearsTags(t *testing.T) { m := &Mark{ - ID: GenerateID("https://example.com"), - Name: "Old", - URL: "https://example.com", - Tags: []string{"a", "b"}, + ID: GenerateID("https://example.com"), + Title: "Old", + URL: "https://example.com", + Tags: []string{"a", "b"}, } - if err := m.UpdateMark("", "", []string{}); err != nil { + if err := m.Update("", "", []string{}); err != nil { t.Fatalf("UpdateMark error: %v", err) } if len(m.Tags) != 0 { t.Errorf("Tags = %v, want empty", m.Tags) } - if err := m.UpdateMark("", "", nil); err != nil { - t.Fatalf("UpdateMark nil error: %v", err) + if err := m.Update("", "", nil); err != nil { + t.Fatalf("Update nil error: %v", err) } if len(m.Tags) != 0 { t.Errorf("Tags = %v, want still empty after nil", m.Tags) @@ -912,7 +946,7 @@ func TestMarshalCatalog(t *testing.T) { name string format string want string - wantNil bool + wantErr bool }{ { name: "json", @@ -925,24 +959,29 @@ func TestMarshalCatalog(t *testing.T) { want: "name = \"example\"\ntags = [\"a\", \"b\"]\n", }, { - name: "empty format returns nil", + name: "empty format", format: "", - wantNil: true, + wantErr: true, + }, + { + name: "unknown format", + format: "yaml", + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := MarshalCatalog(item, tt.format) - if err != nil { - t.Fatalf("MarshalCatalog error: %v", err) - } - if tt.wantNil { - if got != nil { - t.Errorf("got %q, want nil", got) + if tt.wantErr { + if err == nil { + t.Fatalf("MarshalCatalog(%q) expected error, got %q", tt.format, got) } return } + if err != nil { + t.Fatalf("MarshalCatalog error: %v", err) + } if string(got) != tt.want { t.Errorf("got %q, want %q", got, tt.want) } diff --git a/pkg/catalog/doctor_test.go b/pkg/catalog/doctor_test.go index 266476e..1198da8 100644 --- a/pkg/catalog/doctor_test.go +++ b/pkg/catalog/doctor_test.go @@ -117,7 +117,7 @@ func TestIndexStaleFiles(t *testing.T) { // Modifying the file marks it stale again. s.Collections["golang"].Marks = append(s.Collections["golang"].Marks, - &book.Mark{ID: book.GenerateID("https://example.com"), Name: "New", URL: "https://example.com"}) + &book.Mark{ID: book.GenerateID("https://example.com"), Title: "New", URL: "https://example.com"}) writeShelfFile(t, paths, s) stale, err = ix.StaleFiles(paths) if err != nil { diff --git a/pkg/catalog/index.go b/pkg/catalog/index.go index 2f70a9f..55aeee7 100644 --- a/pkg/catalog/index.go +++ b/pkg/catalog/index.go @@ -344,7 +344,9 @@ func (ix *Index) CollectionNames(shelfName string) ([]string, error) { } // Collection returns a reconstructed collection (with marks and tags) for the -// named shelf and collection. Soft-deleted marks are excluded. +// named shelf and collection. Soft-deleted marks are excluded. The returned +// collection and its marks have their Shelf and Collection back-pointers wired, +// matching the BookShelves finders. func (ix *Index) Collection(shelfName, collectionName string) (*book.Collection, error) { var shelfID string err := ix.db.QueryRow(`SELECT shelf_id FROM shelves WHERE name = ?`, shelfName).Scan(&shelfID) @@ -355,7 +357,8 @@ func (ix *Index) Collection(shelfName, collectionName string) (*book.Collection, return nil, err } - col := &book.Collection{} + shelf := &book.Shelf{ID: shelfID, Name: shelfName} + col := &book.Collection{Shelf: shelf} err = ix.db.QueryRow(` SELECT collection_id, name, description, created_at, updated_at FROM collections @@ -379,8 +382,8 @@ func (ix *Index) Collection(shelfName, collectionName string) (*book.Collection, var marks []*book.Mark for rows.Next() { - m := &book.Mark{} - if err := rows.Scan(&m.ID, &m.Name, &m.URL, &m.CreatedAt, &m.UpdatedAt, &m.DeletedAt); err != nil { + m := &book.Mark{Shelf: shelf, Collection: col} + if err := rows.Scan(&m.ID, &m.Title, &m.URL, &m.CreatedAt, &m.UpdatedAt, &m.DeletedAt); err != nil { _ = rows.Close() return nil, err } @@ -721,10 +724,10 @@ func insertShelf(tx *sql.Tx, s *book.Shelf) error { if _, err := tx.Exec(` INSERT INTO marks (catalog_id, collection_id, title, url, created_at, updated_at, deleted_at) VALUES (?, ?, ?, ?, ?, ?, ?)`, - m.ID, c.ID, m.Name, m.URL, m.CreatedAt, m.UpdatedAt, m.DeletedAt); err != nil { + m.ID, c.ID, m.Title, m.URL, m.CreatedAt, m.UpdatedAt, m.DeletedAt); err != nil { return err } - if _, err := tx.Exec(`INSERT INTO marks_fts (title, url, catalog_id) VALUES (?, ?, ?)`, m.Name, m.URL, m.ID); err != nil { + if _, err := tx.Exec(`INSERT INTO marks_fts (title, url, catalog_id) VALUES (?, ?, ?)`, m.Title, m.URL, m.ID); err != nil { return err } for _, tag := range m.Tags { diff --git a/pkg/catalog/index_test.go b/pkg/catalog/index_test.go index d12fba9..c218caf 100644 --- a/pkg/catalog/index_test.go +++ b/pkg/catalog/index_test.go @@ -45,8 +45,8 @@ func sampleShelf() *book.Shelf { Name: "golang", Description: "go links", Marks: []*book.Mark{ - {ID: book.GenerateID("https://go.dev"), Name: "The Go Programming Language", URL: "https://go.dev", Tags: []string{"lang", "official"}}, - {ID: book.GenerateID("https://pkg.go.dev"), Name: "Golang patterns", URL: "https://pkg.go.dev", Tags: []string{"docs"}}, + {ID: book.GenerateID("https://go.dev"), Title: "The Go Programming Language", URL: "https://go.dev", Tags: []string{"lang", "official"}}, + {ID: book.GenerateID("https://pkg.go.dev"), Title: "Golang patterns", URL: "https://pkg.go.dev", Tags: []string{"docs"}}, }, }, }, @@ -181,7 +181,7 @@ func TestSyncIncrementalAndPrune(t *testing.T) { // Add a mark and rewrite: should reindex exactly one file. s := sampleShelf() s.Collections["golang"].Marks = append(s.Collections["golang"].Marks, - &book.Mark{ID: book.GenerateID("https://example.com"), Name: "Example", URL: "https://example.com", Tags: []string{"misc"}}) + &book.Mark{ID: book.GenerateID("https://example.com"), Title: "Example", URL: "https://example.com", Tags: []string{"misc"}}) writeShelfFile(t, paths, s) report, err = ix.Sync(paths) @@ -378,8 +378,8 @@ func TestSearchExcludesSoftDeleted(t *testing.T) { ID: book.GenerateCollectionID("work", "golang"), Name: "golang", Marks: []*book.Mark{ - {ID: book.GenerateID("https://go.dev"), Name: "The Go Programming Language", URL: "https://go.dev", Tags: []string{"lang"}}, - {ID: book.GenerateID("https://pkg.go.dev"), Name: "Golang patterns", URL: "https://pkg.go.dev", Tags: []string{"docs"}, DeletedAt: book.NowTimestamp()}, + {ID: book.GenerateID("https://go.dev"), Title: "The Go Programming Language", URL: "https://go.dev", Tags: []string{"lang"}}, + {ID: book.GenerateID("https://pkg.go.dev"), Title: "Golang patterns", URL: "https://pkg.go.dev", Tags: []string{"docs"}, DeletedAt: book.NowTimestamp()}, }, }, }, @@ -418,8 +418,8 @@ func TestDeletedMarks(t *testing.T) { ID: book.GenerateCollectionID("work", "golang"), Name: "golang", Marks: []*book.Mark{ - {ID: book.GenerateID("https://go.dev"), Name: "The Go Programming Language", URL: "https://go.dev", Tags: []string{"lang"}}, - {ID: book.GenerateID("https://pkg.go.dev"), Name: "Golang patterns", URL: "https://pkg.go.dev", Tags: []string{"docs"}, DeletedAt: book.NowTimestamp()}, + {ID: book.GenerateID("https://go.dev"), Title: "The Go Programming Language", URL: "https://go.dev", Tags: []string{"lang"}}, + {ID: book.GenerateID("https://pkg.go.dev"), Title: "Golang patterns", URL: "https://pkg.go.dev", Tags: []string{"docs"}, DeletedAt: book.NowTimestamp()}, }, }, }, diff --git a/pkg/catalog/toml.go b/pkg/catalog/toml.go index 3df0961..941000c 100644 --- a/pkg/catalog/toml.go +++ b/pkg/catalog/toml.go @@ -19,7 +19,7 @@ func VerifyExists(filename string) (bool, error) { } if errors.Is(err, os.ErrNotExist) { - return false, nil // File does exist + return false, nil // File does not exist } return false, err // remaining errors } @@ -94,7 +94,7 @@ func EnsureConfig(c *book.Config) error { return nil } if !c.Autoconfirm { - return fmt.Errorf("%s", fmt.Sprintf("set --confirm to create config file %s", c.ConfigFile)) + return fmt.Errorf("config file %s does not exist; set --confirm to create it", c.ConfigFile) } fileCfg := &book.FileConfig{ diff --git a/pkg/catalog/toml_test.go b/pkg/catalog/toml_test.go index ec5c1e0..1dec0f2 100644 --- a/pkg/catalog/toml_test.go +++ b/pkg/catalog/toml_test.go @@ -247,9 +247,9 @@ func TestCreateTOMLAtomic(t *testing.T) { Description: "general collection", Marks: []*book.Mark{ { - Name: "Example", - URL: "https://example.com", - Tags: []string{"demo"}, + Title: "Example", + URL: "https://example.com", + Tags: []string{"demo"}, }, }, }, diff --git a/pkg/web/web.go b/pkg/web/web.go index d302f8f..69b446e 100644 --- a/pkg/web/web.go +++ b/pkg/web/web.go @@ -18,7 +18,10 @@ import ( // ErrTitleUnavailable is returned when a page title cannot be fetched // automatically (e.g. HTTP 403 or an empty tag). Callers should // prompt the user to enter a title manually. -var ErrTitleUnavailable = errors.New("couldn't fetch title") +var ErrTitleUnavailable = errors.New("web: couldn't fetch title") + +// ErrNotFound is returned when the page responds with HTTP 404. +var ErrNotFound = errors.New("web: not found") // OpenURL opens the given URL in the default browser. func OpenURL(url string) error { @@ -61,8 +64,8 @@ func WebsiteTitle(ctx context.Context, url string) (string, error) { case http.StatusForbidden: return "", ErrTitleUnavailable case http.StatusNotFound: - return "", fmt.Errorf("betta check yerself - that's a 4oh4!\n%s", url) + return "", fmt.Errorf("fetch title: %w: %s", ErrNotFound, url) default: - return "", fmt.Errorf("unchecked error: %d", res.StatusCode) + return "", fmt.Errorf("fetch title: unexpected status %d", res.StatusCode) } } diff --git a/pkg/web/web_test.go b/pkg/web/web_test.go index f25de43..e9171b8 100644 --- a/pkg/web/web_test.go +++ b/pkg/web/web_test.go @@ -17,6 +17,7 @@ func TestWebsiteTitle(t *testing.T) { wantTitle string wantErr bool wantErrTitleUnavailable bool + wantErrNotFound bool }{ { name: "OK with title", @@ -50,11 +51,12 @@ func TestWebsiteTitle(t *testing.T) { wantErrTitleUnavailable: true, }, { - name: "NotFound", - status: http.StatusNotFound, - body: "", - wantTitle: "", - wantErr: true, + name: "NotFound", + status: http.StatusNotFound, + body: "", + wantTitle: "", + wantErr: true, + wantErrNotFound: true, }, { name: "ServerError", @@ -86,6 +88,9 @@ func TestWebsiteTitle(t *testing.T) { if tt.wantErrTitleUnavailable && !errors.Is(gotErr, ErrTitleUnavailable) { t.Errorf("expected ErrTitleUnavailable, got %v", gotErr) } + if tt.wantErrNotFound && !errors.Is(gotErr, ErrNotFound) { + t.Errorf("expected ErrNotFound, got %v", gotErr) + } }) } }