From b6b0976c194649c30863ff6d449b3292682e0650 Mon Sep 17 00:00:00 2001 From: Anton Vakhrushev Date: Mon, 27 Jul 2026 21:16:29 +0300 Subject: [PATCH] =?UTF-8?q?=D0=B8=D1=81=D0=BF=D1=80=D0=B0=D0=B2=D0=BB?= =?UTF-8?q?=D0=B5=D0=BD=D1=8B=20=D0=BD=D0=B0=D1=85=D0=BE=D0=B4=D0=BA=D0=B8?= =?UTF-8?q?=20=D1=80=D0=B5=D0=B2=D1=8C=D1=8E=20=D0=BF=D1=80=D0=BE=D0=B5?= =?UTF-8?q?=D0=BA=D1=82=D0=BD=D0=BE=D0=B9=20=D1=81=D1=82=D0=BE=D1=80=D0=BE?= =?UTF-8?q?=D0=BD=D1=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - манифест читается так, как записан: решётка внутри строки не открывает комментарий, скобка внутри комментария не закрывает массив, имя внутри комментария не становится подпиской; новый ключ встаёт после массива, а не внутрь него - всё записываемое проходит через manifest.Quote — обратный слэш в пути делал файл, который инструмент сам не читает - маркер локальной части переехал в doc и пропускает огороженные блоки: процитированный в примере маркер больше не считается границей, а копия без маркера не перезаписывается молча - лишний позиционный аргумент отсекается: flag прекращал разбор и прятал флаги после себя, из-за чего pull, list и check игнорировали --for - заведены тесты проверок копий, включая молчание на исправной копии --- CLAUDE.md | 29 ++- internal/check/copies.go | 41 ++--- internal/check/copies_test.go | 177 ++++++++++++++++++ internal/check/form.go | 2 +- internal/cli/add.go | 16 +- internal/cli/check.go | 51 ++++-- internal/cli/cli.go | 26 +++ internal/cli/init.go | 16 +- internal/cli/list.go | 3 + internal/cli/project.go | 29 ++- internal/cli/project_test.go | 140 ++++++++++++++ internal/cli/pull.go | 3 + internal/cli/suitecheck.go | 43 +++-- internal/cli/suitelist.go | 14 -- internal/doc/doc.go | 43 +++++ internal/manifest/edit.go | 37 +++- internal/manifest/editlist.go | 281 ++++++++++++++++++++++------- internal/manifest/editlist_test.go | 96 ++++++++++ internal/project/project.go | 54 +++--- 19 files changed, 904 insertions(+), 197 deletions(-) create mode 100644 internal/check/copies_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 8dabb83..963efab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -68,7 +68,14 @@ internal/cli команды, диалог, два режима заголовка до следующего заголовка любого уровня. Метка открывает блок только первой в абзаце и полужирным. Огороженные блоки кода исключаются везде; инлайн-код вырезается там, где ищутся ссылки, и не вырезается там, где ищутся - пути канона. + пути канона. Маркер локальной части — то же самое: `doc.LocalMarker` один на + весь инструмент, `doc.Marker()` пропускает огороженные блоки, потому что + конвенция о ведении копий этот маркер цитирует. +- **Манифест читается так, как он записан.** Решётка внутри строки не открывает + комментарий, скобка внутри комментария не закрывает массив, имя внутри + комментария не подписка. Регуляркой по сырым строкам это не берётся — + `splitComment` и `scanCode` в `internal/manifest`. Превращение комментария в + данные — единственная ошибка, из которой нет дороги назад. - **Уровень называется ссылкой, а не путём.** Проект ссылается на набор, набор на язык; `source.Ref` разбирает ссылку, `source.Open` отдаёт директорию, которую можно читать. Транспортов два, но `Kind` — перечисление, а не булево: @@ -114,7 +121,16 @@ internal/cli команды, диалог, два режима - **Целостность набора проверяется локально.** Когда `[language] source` заполнен, `suite check` не тянет описание языка и пропускает проверки документов о языке (META-30 в том числе), проверяя вместо этого саму ссылку. - Проверка гоняется на каждой правке и в сеть ходить не должна. + Проверка гоняется на каждой правке и в сеть ходить не должна. Пропуск + объявляется строкой в выводе: молча не выполненная проверка читается ровно + как пройденная. +- **Всё, что попадает в манифест, проходит через `manifest.Quote`.** Обратный + слэш в пути — обычный случай, на котором инструмент перестаёт читать файл, + который сам записал. +- **Позиционный аргумент отсекается явно.** `flag` прекращает разбор на первом + не-флаге, поэтому лишний аргумент не просто лежит без дела — он прячет все + флаги после себя. `noStrayArgs` в командах без позиционных, ручное снятие + темы с головы в `convy add`. ## Проверки @@ -128,7 +144,8 @@ internal/cli команды, диалог, два режима Новая проверка заводится вместе с двумя тестами: что она срабатывает и что она **молчит** там, где не должна. Второй важнее: проверка, краснеющая на исправном файле, выключается целиком. Ложные срабатывания собраны в -`TestNoFalsePositives`. +`TestNoFalsePositives` для набора и в `TestCopiesAreSilentOnASoundCopy` для +копий. Перед тем как заводить проверку, стоит прогнать её замысел по живому канону (`dev-conventions`): если она покраснеет на исправном наборе, замысел неверен. @@ -152,6 +169,12 @@ internal/cli команды, диалог, два режима - `convy add` пишет подписку после сборки — если сборка прошла, а запись в манифест упала, копия останется неучтённой. Обратный порядок хуже: подписка без файла отправляет следующий `pull` искать то, чего не делали. +- Директории компонентов сверяются на равенство, а не на вложенность. Компонент + в `docs` и компонент в `docs/sub` манифест пропустит; `convy check` от + двойных находок защищён отдельно (`distinct`). +- `lang.Recognize` при отсутствии словаря с совпавшим номером версии отдаёт + первого кандидата, у которого совпали слова. Пока версия в реестре одна, это + безвредно; со второй версией того же естественного языка станет неверно. ## Тесты diff --git a/internal/check/copies.go b/internal/check/copies.go index c4a3053..9ddfc13 100644 --- a/internal/check/copies.go +++ b/internal/check/copies.go @@ -19,9 +19,10 @@ import ( // below it takes a prefix on X. // CopyMarker is the boundary between what the suite wrote and what the -// repository wrote. It is passed in rather than known here: the marker belongs -// to the model of assembly, and the checks merely respect it. -const CopyMarker = "" +// repository wrote. It is one constant, defined next to the parsing that has to +// respect it: two of them would drift apart in silence, and each half of the +// tool would then read a different file. +const CopyMarker = doc.LocalMarker // Copy checks one assembled convention. func Copy(d *doc.Document, rep *Report) { @@ -34,8 +35,12 @@ func Copy(d *doc.Document, rep *Report) { } d.Blocks(v) - marker := markerLine(d) - checkMarker(d, marker, rep) + markers := d.Markers() + marker := 0 + if len(markers) > 0 { + marker = markers[0] + } + checkMarker(d, markers, rep) checkCopyHeadings(d, marker, rep) checkHeadingHierarchy(d, rep) for _, prefix := range prefixes(d) { @@ -87,28 +92,18 @@ func recognize(d *doc.Document, rep *Report) (lang.Vocabulary, bool) { return v, true } -// markerLine finds the line the local marker stands on, or zero. -func markerLine(d *doc.Document) int { - for n := 1; n <= d.Len(); n++ { - if strings.TrimSpace(d.Line(n)) == CopyMarker { - return n - } - } - return 0 -} - -func checkMarker(d *doc.Document, marker int, rep *Report) { - if marker == 0 { +// checkMarker checks the boundary of the local part. A marker quoted inside a +// fenced block is not one — a convention about keeping copies carries such a +// quotation — and the parser has already left those out. +func checkMarker(d *doc.Document, markers []int, rep *Report) { + if len(markers) == 0 { rep.Errorf(Spread, d.Path, d.Len(), "the copy carries no %s marker: there is nowhere to write a derogation, and a reassembly would overwrite whatever was written instead", CopyMarker) return } - for n := marker + 1; n <= d.Len(); n++ { - if strings.TrimSpace(d.Line(n)) == CopyMarker { - rep.Errorf(Spread, d.Path, n, - "the copy carries a second %s marker: the marker is one, and everything below the first belongs to the repository", CopyMarker) - return - } + if len(markers) > 1 { + rep.Errorf(Spread, d.Path, markers[1], + "the copy carries a second %s marker: the marker is one, and everything below the first belongs to the repository", CopyMarker) } } diff --git a/internal/check/copies_test.go b/internal/check/copies_test.go new file mode 100644 index 0000000..266d553 --- /dev/null +++ b/internal/check/copies_test.go @@ -0,0 +1,177 @@ +package check_test + +import ( + "strings" + "testing" + + "git.vakhrushev.me/av/convy/internal/check" + "git.vakhrushev.me/av/convy/internal/doc" +) + +// A copy is checked without a suite next to it: no manifest, no prefix of its +// own, no path back to where it came from. These fixtures are therefore written +// out whole rather than assembled — what the check sees is what a consuming +// repository holds. + +const copyVersionLine = `Ключевые слова ДОЛЖЕН, НЕ ДОЛЖЕН, СЛЕДУЕТ, НЕ СЛЕДУЕТ, ДОПУСКАЕТСЯ и метки +ПОЧЕМУ, ПРИМЕРЫ, МЕХАНИЗИРОВАНО и СНЯТО толкуются как описано в языке +конвенций версии 1 — тогда и только тогда, когда написаны заглавными.` + +// soundCopy is what the assembler writes: two layers, the second a section of +// the first, a local part with a rule of the repository in it. +const soundCopy = `--- +origin: time +--- + +# Время + +Как приложение записывает моменты. + +` + copyVersionLine + ` + +## Правила + +### TIME-1. Момент записывается в UTC + +**ДОЛЖЕН.** Момент времени записывается с суффиксом Z. + +**ПОЧЕМУ.** Без явного смещения не видно, в какой зоне запись сделана. + +### TIME-2. Ширина строки фиксируется + +**СНЯТО 2026-07-27.** ширина следует из TIME-1. + +## Время: реализация на Go + +Как базовый слой выполняется в Go-коде. + +### Правила + +#### GTIM-1. «Сейчас» берётся у слоя хранилища + +**ДОЛЖЕН.** Текущее время приходит из ` + "`store.Now()`" + `. + +**ПОЧЕМУ.** Единая точка даёт гарантированный UTC. + + + +TIME-1 — МЕХАНИЗИРОВАНО: ` + "`internal/archrules`" + `. +Ссылка на чужую тему: SLOG-4 — эта копия её не держит. + +### XTIM-1. Часы в тестах замораживаются + +**ДОЛЖЕН.** Тест берёт время у подменённого ` + "`store.Now`" + `. + +**ПОЧЕМУ.** Плавающее время делает падение теста невоспроизводимым. +` + +func checkCopy(t *testing.T, body string) *check.Report { + t.Helper() + d, err := doc.Parse("docs/conventions/time.md", body) + if err != nil { + t.Fatalf("parsing the fixture: %v", err) + } + return check.Copies([]*doc.Document{d}) +} + +// The one that matters most: a check that reddens on a sound file is a check +// that gets switched off whole. +func TestCopiesAreSilentOnASoundCopy(t *testing.T) { + rep := checkCopy(t, soundCopy) + if len(rep.Findings()) == 0 { + return + } + var b strings.Builder + for _, f := range rep.Findings() { + b.WriteString(" " + f.Msg + "\n") + } + t.Errorf("a sound copy produced findings:\n%s", b.String()) +} + +// A convention about keeping copies quotes the marker in an example. The +// quotation is markup shown, not markup meant. +func TestCopiesReadNoMarkerInsideAFencedBlock(t *testing.T) { + quoting := strings.Replace(soundCopy, "## Время: реализация на Go", + "## Пример\n\n"+"```markdown\n\n```\n\n## Время: реализация на Go", 1) + + rep := checkCopy(t, quoting) + for _, f := range rep.Findings() { + if strings.Contains(f.Msg, "marker") { + t.Errorf("a quoted marker was taken for the boundary: %s", f.Msg) + } + } +} + +func TestCopiesCatchWhatOnlyACopyCanGetWrong(t *testing.T) { + cases := []struct { + name string + edit func(string) string + want string + }{{ + name: "no origin key", + edit: func(s string) string { return strings.Replace(s, "origin: time", "topic: time", 1) }, + want: "no origin key", + }, { + name: "a key of a suite file left in the front matter", + edit: func(s string) string { + return strings.Replace(s, "origin: time", "origin: time\nextends: arch/time.md", 1) + }, + want: "key \"extends\" of a suite file", + }, { + name: "no marker at all", + edit: func(s string) string { return strings.Replace(s, "", "", 1) }, + want: "carries no marker", + }, { + name: "a second marker", + edit: func(s string) string { return s + "\n\n" }, + want: "second marker", + }, { + name: "a rule of the repository above the marker", + edit: func(s string) string { + return strings.Replace(s, "### TIME-2.", "### XTIM-9. Своё правило\n\n**ДОЛЖЕН.** Своя норма.\n\n**ПОЧЕМУ.** Своя причина.\n\n### TIME-2.", 1) + }, + want: "would wipe it", + }, { + name: "a rule of the suite below the marker", + edit: func(s string) string { return strings.Replace(s, "### XTIM-1.", "### ZTIM-1.", 1) }, + want: "takes a prefix the suite could hand out", + }, { + name: "a gap in the numbering of one of the prefixes", + edit: func(s string) string { return strings.Replace(s, "#### GTIM-1.", "#### GTIM-2.", 1) }, + want: "numbering is not contiguous", + }, { + name: "a reference to a rule the file holds no such number of", + edit: func(s string) string { + return strings.Replace(s, "TIME-1 — МЕХАНИЗИРОВАНО", "TIME-9 — МЕХАНИЗИРОВАНО", 1) + }, + want: "points at a rule this file does not hold", + }, { + name: "no language version line", + edit: func(s string) string { + return strings.Replace(s, copyVersionLine, "Просто вступление.", 1) + }, + want: "no language version line", + }, { + name: "a word of another vocabulary", + edit: func(s string) string { + return strings.Replace(s, "**ДОЛЖЕН.** Момент", "**MUST.** Момент", 1) + }, + want: "belongs to the \"en\" vocabulary", + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rep := checkCopy(t, tc.edit(soundCopy)) + for _, f := range rep.Findings() { + if strings.Contains(f.Msg, tc.want) { + return + } + } + var b strings.Builder + for _, f := range rep.Findings() { + b.WriteString(" " + f.Msg + "\n") + } + t.Errorf("nothing said %q; the findings were:\n%s", tc.want, b.String()) + }) + } +} diff --git a/internal/check/form.go b/internal/check/form.go index 696d270..fcf25d6 100644 --- a/internal/check/form.go +++ b/internal/check/form.go @@ -269,7 +269,7 @@ func checkVersionLine(v lang.Vocabulary, d *doc.Document, rep *Report) (from, to version := strconv.Itoa(v.Version) if !containsNumber(p.Text(), version) { rep.Errorf(Form, d.Path, p.Start, - "the language version line does not name version %s declared by the suite manifest", version) + "the language version line does not name version %s, the version this document is read by", version) } return p.Start, p.End } diff --git a/internal/cli/add.go b/internal/cli/add.go index a479de8..646453c 100644 --- a/internal/cli/add.go +++ b/internal/cli/add.go @@ -41,12 +41,9 @@ func runAdd(env Env, args []string) ExitCode { } topic = *topicFlag } - if rest := fs.Args(); len(rest) > 0 { - if topic != "" && topic != rest[0] { - fmt.Fprintf(env.Err, "the topic is named twice and differently: %q and %q\n", topic, rest[0]) - return Usage - } - topic = rest[0] + if left := fs.Args(); len(left) > 0 { + fmt.Fprintf(env.Err, "convy add takes one topic, and %q came after it as well\n", left[0]) + return Usage } o, code := openProject(env, *root) @@ -70,12 +67,11 @@ func runAdd(env Env, args []string) ExitCode { fmt.Fprintln(env.Err, "convy add without arguments asks which topic, and there is no terminal to ask on; name the topic as an argument") return Usage } - answer, err := newDialogue(env).ask(topicField(o, c)) - if err != nil { - fmt.Fprintln(env.Err, "\ninterrupted, nothing was written") + given := map[string]string{} + if err := askAll(env, []Field{topicField(o, c)}, given); err != nil { return Usage } - topic = answer + topic = given["topic"] } if !o.Suite.Manifest.TopicLive(topic) { diff --git a/internal/cli/check.go b/internal/cli/check.go index 2c26ad9..77a85aa 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -11,7 +11,6 @@ import ( "git.vakhrushev.me/av/convy/internal/check" "git.vakhrushev.me/av/convy/internal/doc" - "git.vakhrushev.me/av/convy/internal/manifest" "git.vakhrushev.me/av/convy/internal/project" ) @@ -29,13 +28,19 @@ func runCheck(env Env, args []string) ExitCode { if err := fs.Parse(args); err != nil { return Usage } + if code := noStrayArgs(env, "convy check", fs.Args()); code != OK { + return code + } dir, code := projectRoot(env, *root) if code != OK { return code } - m, err := manifest.LoadProject(dir) - if err != nil { + m, code := loadProject(env, dir) + if code != OK { + return code + } + if err := distinctDirs(m); err != nil { fmt.Fprintln(env.Err, err) return Usage } @@ -47,10 +52,16 @@ func runCheck(env Env, args []string) ExitCode { var docs []*doc.Document var broken []error for _, name := range names { - found, errs := copies(dir, m.Components[name].Dir) + c := m.Components[name] + if c.Dir == "" { + fmt.Fprintf(env.Err, "the component %q names no dir, and there is nothing to look in\n", name) + return Usage + } + found, errs := copies(dir, c.Dir) docs = append(docs, found...) broken = append(broken, errs...) } + docs = distinct(docs) rep := check.Copies(docs) for _, err := range broken { @@ -104,23 +115,25 @@ func copies(root, dir string) ([]*doc.Document, []error) { return docs, broken } +// distinct drops a document reached through two components. Directories are +// checked for equality before this, but one may still lie inside another, and a +// finding printed twice reads as two. +func distinct(docs []*doc.Document) []*doc.Document { + seen := make(map[string]bool, len(docs)) + out := docs[:0] + for _, d := range docs { + if seen[d.Path] { + continue + } + seen[d.Path] = true + out = append(out, d) + } + return out +} + func printCopyReport(w io.Writer, rep *check.Report, files, comps int, quiet bool) { findings := rep.Findings() - current := "" - for _, f := range findings { - if f.Path != current { - if current != "" { - fmt.Fprintln(w) - } - fmt.Fprintf(w, "%s\n", f.Path) - current = f.Path - } - where := "" - if f.Line > 0 { - where = fmt.Sprintf(":%d", f.Line) - } - fmt.Fprintf(w, " %s%s %s [%s]\n", f.Severity, where, f.Msg, f.Family) - } + printFindings(w, findings) if quiet { return diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 0e56a11..45091c3 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -11,6 +11,7 @@ import ( "fmt" "io" "os" + "strings" ) // ExitCode is the exit status of the process. @@ -118,6 +119,31 @@ everything at once and ask nothing — that mode is for agents and scripts. `) } +// noStrayArgs turns down an argument the command has no place for. The flag +// package stops parsing at the first argument that is not a flag, so a stray one +// does not merely sit there unused — it hides every flag written after it, and +// the command then does something other than what was asked in silence. +func noStrayArgs(env Env, name string, rest []string) ExitCode { + if len(rest) == 0 { + return OK + } + fmt.Fprintf(env.Err, "%s takes no argument, and %q was given; a component is named by --for\n", name, rest[0]) + return Usage +} + +// split reads a comma-separated list off the command line. An axis of a +// component is a list — a component may sit on two stacks at once — and one +// flag repeated is worse to type than one flag with commas in it. +func split(value string) []string { + var out []string + for _, part := range strings.Split(value, ",") { + if part = strings.TrimSpace(part); part != "" { + out = append(out, part) + } + } + return out +} + // Main is the entry point of the process. func Main() int { dir, err := os.Getwd() diff --git a/internal/cli/init.go b/internal/cli/init.go index be0c939..2c5edf8 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -28,7 +28,7 @@ const projectSkeleton = `# What this repository takes from a conventions suite. # Where the copies come from: a path on disk, relative to this file or absolute, # or a git repository over http or https. A trailing #branch, #tag or #commit # pins a revision. -source = "%s" +source = %s # ─── Components ───────────────────────────────────────────────────────────── # @@ -54,6 +54,9 @@ func runInit(env Env, args []string) ExitCode { if err := fs.Parse(args); err != nil { return Usage } + if code := noStrayArgs(env, "convy init", fs.Args()); code != OK { + return code + } where := *root if where == "" { @@ -102,7 +105,7 @@ func runInit(env Env, args []string) ExitCode { return Failed } - entries := [][2]string{{"dir", quote(given["dir"])}} + entries := [][2]string{{"dir", manifest.Quote(given["dir"])}} if list := split(given["lang"]); len(list) > 0 { entries = append(entries, [2]string{"lang", array(list)}) } @@ -111,7 +114,7 @@ func runInit(env Env, args []string) ExitCode { } entries = append(entries, [2]string{"topics", "[]"}) - body, err := manifest.AddTable([]byte(fmt.Sprintf(projectSkeleton, given["source"])), + body, err := manifest.AddTable(fmt.Appendf(nil, projectSkeleton, manifest.Quote(given["source"])), "components."+given["component"], entries) if err != nil { fmt.Fprintln(env.Err, err) @@ -179,14 +182,11 @@ func initFields() []Field { }} } -func quote(s string) string { - return `"` + strings.ReplaceAll(s, `"`, `\"`) + `"` -} - +// array writes a TOML array of strings. func array(values []string) string { parts := make([]string, len(values)) for i, v := range values { - parts[i] = quote(v) + parts[i] = manifest.Quote(v) } return "[" + strings.Join(parts, ", ") + "]" } diff --git a/internal/cli/list.go b/internal/cli/list.go index ee8dec6..e28f62e 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -23,6 +23,9 @@ func runList(env Env, args []string) ExitCode { if err := fs.Parse(args); err != nil { return Usage } + if code := noStrayArgs(env, "convy list", fs.Args()); code != OK { + return code + } o, code := openProject(env, *root) if code != OK { diff --git a/internal/cli/project.go b/internal/cli/project.go index 2f4e0fd..8f60557 100644 --- a/internal/cli/project.go +++ b/internal/cli/project.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "git.vakhrushev.me/av/convy/internal/manifest" "git.vakhrushev.me/av/convy/internal/source" @@ -40,13 +41,9 @@ func openProject(env Env, root string) (*opened, ExitCode) { if code != OK { return nil, code } - m, err := manifest.LoadProject(dir) - if err != nil { - fmt.Fprintln(env.Err, err) - return nil, Usage - } - for _, key := range m.Undecoded { - fmt.Fprintf(env.Err, "warning: %s: the key %s is unknown to the tool\n", m.Path, key) + m, code := loadProject(env, dir) + if code != OK { + return nil, code } if m.Source == "" { fmt.Fprintf(env.Err, "%s names no source: a copy comes from a suite, and the manifest is where the suite is named\n", m.Path) @@ -93,6 +90,22 @@ func openProject(env Env, root string) (*opened, ExitCode) { return o, OK } +// loadProject reads the project manifest and says what it did not understand. +// A typo in a key costs a whole component: `dyr` instead of `dir` leaves the +// component pointing at the root of the repository, and nothing else would say +// so. +func loadProject(env Env, dir string) (*manifest.Project, ExitCode) { + m, err := manifest.LoadProject(dir) + if err != nil { + fmt.Fprintln(env.Err, err) + return nil, Usage + } + for _, key := range m.Undecoded { + fmt.Fprintf(env.Err, "warning: %s: the key %s is unknown to the tool\n", m.Path, key) + } + return m, OK +} + // projectRoot finds the manifest of the project. A project command typed inside // a suite does not do anything at a guess: it says where it is and names the // command of that level. @@ -133,7 +146,7 @@ func componentOf(env Env, m *manifest.Project, name string) (string, manifest.Co func components(env Env, m *manifest.Project, name string) ([]string, ExitCode) { if name != "" { if _, ok := m.Components[name]; !ok { - fmt.Fprintf(env.Err, "the project declares no component %q\n", name) + fmt.Fprintf(env.Err, "the project declares no component %q; it declares: %s\n", name, strings.Join(m.Names(), ", ")) return nil, Usage } return []string{name}, OK diff --git a/internal/cli/project_test.go b/internal/cli/project_test.go index 2d55e00..0ac2266 100644 --- a/internal/cli/project_test.go +++ b/internal/cli/project_test.go @@ -1,12 +1,16 @@ package cli_test import ( + "fmt" "os" + "path" "path/filepath" "strings" "testing" + "git.vakhrushev.me/av/convy/internal/check" "git.vakhrushev.me/av/convy/internal/cli" + "git.vakhrushev.me/av/convy/internal/doc" ) // readingGuide stands in for what a suite puts next to its copies: the short @@ -343,6 +347,11 @@ func TestTheLanguageMayLiveApartFromTheSuite(t *testing.T) { } checkClean(t, suiteRoot) + // A check that did not run says so: silence would read as a check passed. + if _, out := run(t, suiteRoot, "", false, "suite", "check"); !strings.Contains(out, "went unchecked") { + t.Errorf("the check did not say it left the language alone:\n%s", out) + } + root := wired(t, suiteRoot, "--component", "backend", "--dir", "docs/conventions", "--lang", "go") if code, out := run(t, root, "", false, "add", "time"); code != cli.OK { t.Fatalf("convy add returned %d: %s", code, out) @@ -385,3 +394,134 @@ func TestCheckCatchesALocalRuleAboveTheMarker(t *testing.T) { t.Errorf("the finding does not say what is at stake:\n%s", out) } } + +// copyClean is the project half of the invariant worth the most: what the tool +// writes, the tool accepts — and accepts in silence, warnings included. +func copyClean(t *testing.T, root, dir string) { + t.Helper() + var docs []*doc.Document + entries, err := os.ReadDir(filepath.Join(root, filepath.FromSlash(dir))) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if filepath.Ext(e.Name()) != ".md" { + continue + } + rel := path.Join(dir, e.Name()) + d, err := doc.Load(rel, filepath.Join(root, filepath.FromSlash(rel))) + if err != nil { + t.Fatalf("loading %s: %v", rel, err) + } + if d.Front.Origin != "" { + docs = append(docs, d) + } + } + if len(docs) == 0 { + t.Fatalf("no copy was assembled in %s", dir) + } + rep := check.Copies(docs) + if len(rep.Findings()) == 0 { + return + } + var b strings.Builder + for _, f := range rep.Findings() { + fmt.Fprintf(&b, " %s: %s\n", f.Path, f.Msg) + } + t.Fatalf("the copies the tool assembled do not check clean:\n%s", b.String()) +} + +func TestWhatTheToolAssemblesTheToolAccepts(t *testing.T) { + suiteRoot := subscribable(t) + root := wired(t, suiteRoot, "--component", "backend", "--dir", "docs/conventions", "--lang", "go") + run(t, root, "", false, "add", "time") + run(t, root, "", false, "add", "logging") + copyClean(t, root, "docs/conventions") +} + +// The flag package stops at the first argument that is not a flag, so a stray +// one hides every flag written after it and the command quietly does something +// else than what was asked. +func TestProjectCommandsTurnDownAStrayArgument(t *testing.T) { + suiteRoot := subscribable(t) + root := wired(t, suiteRoot, "--component", "backend", "--dir", "docs/conventions", "--lang", "go") + + for _, args := range [][]string{ + {"pull", "backend"}, + {"list", "backend"}, + {"check", "backend"}, + {"init", "somewhere"}, + } { + t.Run(args[0], func(t *testing.T) { + code, out := run(t, root, "", false, args...) + if code == cli.OK { + t.Fatalf("the stray argument went through:\n%s", out) + } + if !strings.Contains(out, "takes no argument") { + t.Errorf("the refusal does not say what is wrong:\n%s", out) + } + }) + } +} + +// The marker is always left by the assembler, so a copy without one was edited +// by hand — and everything in it would count as suite text to be replaced. +func TestPullRefusesACopyWhoseMarkerIsGone(t *testing.T) { + suiteRoot := subscribable(t) + root := wired(t, suiteRoot, "--component", "backend", "--dir", "docs/conventions", "--lang", "go") + run(t, root, "", false, "add", "time") + + name := filepath.Join(root, "docs", "conventions", "time.md") + body := read(t, root, "docs/conventions/time.md") + body = strings.Replace(body, "", "Заметка, написанная руками.", 1) + if err := os.WriteFile(name, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + + code, out := run(t, root, "", false, "pull") + if code == cli.OK { + t.Fatalf("the pull went ahead over a hand-edited copy:\n%s", out) + } + if !strings.Contains(out, "carries no marker") { + t.Errorf("the refusal does not say why:\n%s", out) + } + if got := read(t, root, "docs/conventions/time.md"); !strings.Contains(got, "Заметка, написанная руками") { + t.Errorf("the hand-written text was destroyed anyway:\n%s", got) + } +} + +// Whatever the tool writes into the manifest it has to read back, and a +// backslash is the ordinary way that fails. +func TestInitWritesAManifestItCanRead(t *testing.T) { + suiteRoot := subscribable(t) + root := wired(t, suiteRoot, "--component", "backend", "--dir", `docs\conventions`) + + code, out := run(t, root, "", false, "list") + if code != cli.OK { + t.Fatalf("the manifest the tool wrote does not parse: %d\n%s", code, out) + } + if !strings.Contains(out, `docs\conventions`) { + t.Errorf("the directory came back changed:\n%s", out) + } +} + +// A typo in a key costs a whole component, and the parser knows about it — so +// every command that reads the manifest has to pass that on. +func TestCheckSaysWhatItDidNotUnderstand(t *testing.T) { + suiteRoot := subscribable(t) + root := wired(t, suiteRoot, "--component", "backend", "--dir", "docs/conventions", "--lang", "go") + + body := read(t, root, ".conventions.toml") + body = strings.Replace(body, `dir = "docs/conventions"`, `dyr = "docs/conventions"`, 1) + if err := os.WriteFile(filepath.Join(root, ".conventions.toml"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + + code, out := run(t, root, "", false, "check") + if !strings.Contains(out, "dyr") { + t.Errorf("the typo in the manifest went unmentioned:\n%s", out) + } + if code == cli.OK { + t.Errorf("a component with no dir was checked anyway:\n%s", out) + } +} diff --git a/internal/cli/pull.go b/internal/cli/pull.go index 7202b4e..bf8992b 100644 --- a/internal/cli/pull.go +++ b/internal/cli/pull.go @@ -21,6 +21,9 @@ func runPull(env Env, args []string) ExitCode { if err := fs.Parse(args); err != nil { return Usage } + if code := noStrayArgs(env, "convy pull", fs.Args()); code != OK { + return code + } o, code := openProject(env, *root) if code != OK { diff --git a/internal/cli/suitecheck.go b/internal/cli/suitecheck.go index 7b05e94..48d7d14 100644 --- a/internal/cli/suitecheck.go +++ b/internal/cli/suitecheck.go @@ -55,21 +55,7 @@ func runSuiteCheck(env Env, args []string) ExitCode { func printReport(w io.Writer, rep *check.Report, s *suite.Suite, quiet bool) { findings := rep.Findings() - current := "" - for _, f := range findings { - if f.Path != current { - if current != "" { - fmt.Fprintln(w) - } - fmt.Fprintf(w, "%s\n", f.Path) - current = f.Path - } - where := "" - if f.Line > 0 { - where = fmt.Sprintf(":%d", f.Line) - } - fmt.Fprintf(w, " %s%s %s [%s]\n", f.Severity, where, f.Msg, f.Family) - } + printFindings(w, findings) if quiet { return @@ -81,6 +67,12 @@ func printReport(w io.Writer, rep *check.Report, s *suite.Suite, quiet bool) { plural(len(s.Docs), "file"), plural(len(s.Manifest.LiveTopics()), "topic"), s.Manifest.Language.Version, s.Manifest.Language.Lang) + // A check that was skipped says so. Reaching the language costs a fetch and + // this check runs on every edit, so the documents about the language go + // unread — and a check silently not run reads exactly like a check passed. + if spec := s.Manifest.Language.Source; spec != "" { + fmt.Fprintf(w, "the language lies at %s and was not fetched: its documents went unchecked\n", spec) + } switch { case rep.Errors() > 0: fmt.Fprintf(w, "errors: %d, warnings: %d\n", rep.Errors(), rep.Warnings()) @@ -98,3 +90,24 @@ func plural(n int, noun string) string { } return fmt.Sprintf("%d %ss", n, noun) } + +// printFindings lays the findings out grouped by file. Both checks print them +// the same way: a finding of the suite and a finding of a copy are read by the +// same person, and two layouts would be two things to learn. +func printFindings(w io.Writer, findings []check.Finding) { + current := "" + for _, f := range findings { + if f.Path != current { + if current != "" { + fmt.Fprintln(w) + } + fmt.Fprintf(w, "%s\n", f.Path) + current = f.Path + } + where := "" + if f.Line > 0 { + where = fmt.Sprintf(":%d", f.Line) + } + fmt.Fprintf(w, " %s%s %s [%s]\n", f.Severity, where, f.Msg, f.Family) + } +} diff --git a/internal/cli/suitelist.go b/internal/cli/suitelist.go index b79bbbc..fbee0ed 100644 --- a/internal/cli/suitelist.go +++ b/internal/cli/suitelist.go @@ -5,7 +5,6 @@ import ( "fmt" "io" "sort" - "strings" "git.vakhrushev.me/av/convy/internal/doc" "git.vakhrushev.me/av/convy/internal/lang" @@ -142,19 +141,6 @@ func listRetired(w io.Writer, s *suite.Suite) { fmt.Fprintln(w, "\nnone of these names is ever handed out again") } -// split reads a comma-separated list off the command line. An axis of a -// component is a list — a component may sit on two stacks at once — and one -// flag repeated is worse to type than one flag with commas in it. -func split(value string) []string { - var out []string - for _, part := range strings.Split(value, ",") { - if part = strings.TrimSpace(part); part != "" { - out = append(out, part) - } - } - return out -} - func sortedKeys(m map[string]string) []string { keys := make([]string, 0, len(m)) for k := range m { diff --git a/internal/doc/doc.go b/internal/doc/doc.go index 7025301..531db74 100644 --- a/internal/doc/doc.go +++ b/internal/doc/doc.go @@ -378,3 +378,46 @@ func StripInline(line string) string { // Len returns the number of lines in the file. func (d *Document) Len() int { return len(d.lines) } + +// LocalMarker is the boundary inside a copy between what the suite wrote and +// what the consuming repository wrote. It is the one piece of markup inside a +// convention that means something to the tool, and it is single and nameless, +// so there is no name to be orphaned by a rename. +// +// It lives here, next to the parsing, because both halves of the tool need it +// and need it read the same way: the assembler to know what to keep, the check +// to know what answers to which rules. Two constants would drift in silence. +const LocalMarker = "" + +// Marker returns the line the local marker stands on, or zero. +// +// A marker inside a fenced block is a quotation of the markup rather than the +// markup itself — a convention about keeping copies would carry one — and +// taking it for the boundary would hand the whole document to the repository. +func (d *Document) Marker() int { + for n := 1; n <= d.Len(); n++ { + if !d.fence[n-1] && strings.TrimSpace(d.lines[n-1]) == LocalMarker { + return n + } + } + return 0 +} + +// Markers counts the local markers outside fenced blocks. +func (d *Document) Markers() []int { + var out []int + for n := 1; n <= d.Len(); n++ { + if !d.fence[n-1] && strings.TrimSpace(d.lines[n-1]) == LocalMarker { + out = append(out, n) + } + } + return out +} + +// Below returns the lines from n to the end, joined. +func (d *Document) Below(n int) string { + if n < 1 || n > d.Len() { + return "" + } + return strings.Join(d.lines[n-1:], "\n") +} diff --git a/internal/manifest/edit.go b/internal/manifest/edit.go index 688714d..9b35f6c 100644 --- a/internal/manifest/edit.go +++ b/internal/manifest/edit.go @@ -135,15 +135,46 @@ func insertionPoint(keys []string, at []int, key string, lines []string, start, } } } - return at[len(at)-1] + 1 + // The entry goes after the last key, and a key is not always one line: an + // array written down a column ends where its bracket closes. Appending + // after the first line of it would land the entry inside the array. + return valueEnd(lines, at[len(at)-1], end) + 1 +} + +// valueEnd returns the last line the value of a key occupies. +func valueEnd(lines []string, at, to int) int { + depth := 0 + for n := at; n < to; n++ { + code, _ := splitComment(lines[n]) + if n == at { + if i := strings.Index(code, "="); i >= 0 { + code = code[i+1:] + } + } + _, next, closeAt := scanCode(code, depth) + if closeAt >= 0 { + return n + } + if depth = next; depth <= 0 { + return n + } + } + return at } // renderEntry writes one key-value line, quoting the key when it is not bare. func renderEntry(key, value string) string { if !bareRe.MatchString(key) { - key = `"` + escape(key) + `"` + key = Quote(key) } - return key + ` = "` + escape(value) + `"` + return key + " = " + Quote(value) +} + +// Quote writes a string the way TOML reads it back. Every value the tool puts +// into a manifest goes through here: a Windows path is the ordinary case where +// a backslash left alone makes the tool unable to read the file it just wrote. +func Quote(s string) string { + return `"` + escape(s) + `"` } func escape(s string) string { diff --git a/internal/manifest/editlist.go b/internal/manifest/editlist.go index 94a3453..66c58c8 100644 --- a/internal/manifest/editlist.go +++ b/internal/manifest/editlist.go @@ -4,7 +4,6 @@ import ( "fmt" "regexp" "slices" - "sort" "strings" ) @@ -13,13 +12,43 @@ import ( // author wrote around the data — which component is what, why a topic is taken // — has to survive the tool touching the file. // -// The shape of the array survives too. An array written on one line stays on -// one line, one written down a column stays a column: reflowing it would make -// every commit that adds a topic look like a rewrite of the file. +// So the array is read as the manifest wrote it, not as a regular expression +// over the raw lines: a # inside a string does not open a comment, a bracket +// inside a comment does not close an array, and a name inside a comment is not +// a subscription. Getting any of the three wrong turns a comment into data, and +// there is no way back from that. +// +// The shape survives too. An array written on one line stays on one line, one +// written down a column stays a column, and the comments keep their places: a +// comment block above a value belongs to that value — the same rule that +// governs a comment above a table — so sorting carries it along, while a +// comment trailing on the same line stays on its line. var arrayKeyRe = regexp.MustCompile(`^(\s*)("[^"]+"|[A-Za-z0-9_-]+)\s*=\s*\[`) -// AddToList appends a value to the array under key in the given table. A table +// element is one value of an array together with what was written around it. +type element struct { + value string + above []string + after string +} + +// array is an array of the manifest, parsed. +type array struct { + indent string + key string + // column says the array was written down a column rather than on one line. + column bool + elems []element + // opening is a comment trailing the opening bracket. + opening string + // dangling holds comment lines standing after the last value. + dangling []string + // tail is whatever follows the closing bracket. + tail string +} + +// AddToList adds a value to the array under key in the given table. A table // that has no such key gets one holding the single value. func AddToList(source []byte, table, key, value string) ([]byte, error) { lines := strings.Split(string(source), "\n") @@ -29,7 +58,7 @@ func AddToList(source []byte, table, key, value string) ([]byte, error) { return nil, fmt.Errorf("the manifest holds no table [%s]", table) } - from, to, found := arrayBounds(lines, start+1, end, key) + from, found := arrayKeyLine(lines, start+1, end, key) if !found { keys, at := tableKeys(lines, start+1, end) insert := insertionPoint(keys, at, key, lines, start, end) @@ -41,21 +70,25 @@ func AddToList(source []byte, table, key, value string) ([]byte, error) { return []byte(strings.Join(out, "\n")), nil } - values := arrayValues(lines[from : to+1]) - if slices.Contains(values, value) { + a, last, ok := readArray(lines, from, end) + if !ok { + return nil, fmt.Errorf("the array %s in [%s] is not closed by a bracket", key, table) + } + if slices.ContainsFunc(a.elems, func(e element) bool { return e.value == value }) { return nil, fmt.Errorf("%s already holds %q", key, value) } - sorted := sort.StringsAreSorted(values) - values = append(values, value) - if sorted { - sort.Strings(values) - } - indent := arrayKeyRe.FindStringSubmatch(lines[from])[1] - return splice(lines, from, to, renderArray(indent, key, values, from != to)), nil + sorted := slices.IsSortedFunc(a.elems, byValue) + a.elems = append(a.elems, element{value: value}) + if sorted { + slices.SortStableFunc(a.elems, byValue) + } + return splice(lines, from, last, a.render()), nil } -// RemoveFromList drops a value from the array under key. +// RemoveFromList drops a value from the array under key. It is the other half +// of AddToList: unsubscribing is not a command yet, and a pair of edits where +// only one direction is written is a pair where the untried direction is wrong. func RemoveFromList(source []byte, table, key, value string) ([]byte, error) { lines := strings.Split(string(source), "\n") @@ -63,22 +96,27 @@ func RemoveFromList(source []byte, table, key, value string) ([]byte, error) { if !ok { return nil, fmt.Errorf("the manifest holds no table [%s]", table) } - from, to, found := arrayBounds(lines, start+1, end, key) + from, found := arrayKeyLine(lines, start+1, end, key) if !found { return nil, fmt.Errorf("the table [%s] holds no key %s", table, key) } + a, last, ok := readArray(lines, from, end) + if !ok { + return nil, fmt.Errorf("the array %s in [%s] is not closed by a bracket", key, table) + } - values := arrayValues(lines[from : to+1]) - i := slices.Index(values, value) + i := slices.IndexFunc(a.elems, func(e element) bool { return e.value == value }) if i < 0 { return nil, fmt.Errorf("%s does not hold %q", key, value) } - values = slices.Delete(values, i, i+1) - - indent := arrayKeyRe.FindStringSubmatch(lines[from])[1] - return splice(lines, from, to, renderArray(indent, key, values, from != to)), nil + // The comment above a value went with it and goes away with it; a comment + // left hanging over the next value would say the wrong thing about it. + a.elems = slices.Delete(a.elems, i, i+1) + return splice(lines, from, last, a.render()), nil } +func byValue(a, b element) int { return strings.Compare(a.value, b.value) } + // AddTable appends a table to the end of the manifest. A new component is a new // table, and it goes last because the order of components is the author's: // there is nothing to sort them by that would mean anything. @@ -122,54 +160,167 @@ func tableBounds(lines []string, table string) (start, end int, ok bool) { return start, end, true } -// arrayBounds finds the first and the last line of the array under key. -func arrayBounds(lines []string, from, to int, key string) (start, end int, ok bool) { +// arrayKeyLine finds the line an array opens on. +func arrayKeyLine(lines []string, from, to int, key string) (int, bool) { for i := from; i < to; i++ { - m := arrayKeyRe.FindStringSubmatch(lines[i]) - if m == nil || strings.Trim(m[2], `"`) != key { + code, _ := splitComment(lines[i]) + m := arrayKeyRe.FindStringSubmatch(code) + if m != nil && strings.Trim(m[2], `"`) == key { + return i, true + } + } + return 0, false +} + +// readArray parses the array opening on line from and returns the line it +// closes on. +func readArray(lines []string, from, to int) (a array, last int, ok bool) { + code, comment := splitComment(lines[from]) + m := arrayKeyRe.FindStringSubmatch(code) + if m == nil { + return a, 0, false + } + a.indent, a.key = m[1], strings.Trim(m[2], `"`) + + open := strings.Index(code, "[") + values, depth, closeAt := scanCode(code[open:], 0) + for _, v := range values { + a.elems = append(a.elems, element{value: v}) + } + if closeAt >= 0 { + a.tail = joinTail(code[open+closeAt:], comment) + return a, from, true + } + a.column = true + a.opening = comment + + var pending []string + for n := from + 1; n < to; n++ { + code, comment := splitComment(lines[n]) + values, next, closeAt := scanCode(code, depth) + depth = next + + if len(values) == 0 && strings.TrimSpace(code) == "" && closeAt < 0 { + if comment != "" { + pending = append(pending, lines[n]) + } continue } - for j := i; j < to; j++ { - if strings.Contains(lines[j], "]") { - return i, j, true + for i, v := range values { + e := element{value: v} + if i == 0 { + e.above, pending = pending, nil + } + if i == len(values)-1 && closeAt < 0 { + e.after = comment + } + a.elems = append(a.elems, e) + } + if closeAt >= 0 { + a.dangling = pending + a.tail = joinTail(code[closeAt:], comment) + return a, n, true + } + } + return a, 0, false +} + +// render writes the array back in the shape it had. +func (a array) render() []string { + quoted := make([]string, len(a.elems)) + for i, e := range a.elems { + quoted[i] = `"` + escape(e.value) + `"` + } + + if !a.column { + line := fmt.Sprintf("%s%s = [%s]", a.indent, a.key, strings.Join(quoted, ", ")) + return []string{appendTail(line, a.tail)} + } + + out := []string{appendTail(a.indent+a.key+" = [", a.opening)} + for i, e := range a.elems { + out = append(out, e.above...) + out = append(out, appendTail(a.indent+" "+quoted[i]+",", e.after)) + } + out = append(out, a.dangling...) + return append(out, appendTail(a.indent+"]", a.tail)) +} + +func appendTail(line, tail string) string { + if tail == "" { + return line + } + return line + " " + tail +} + +func joinTail(rest, comment string) string { + return strings.TrimSpace(strings.TrimSpace(rest) + " " + comment) +} + +// splitComment cuts a line into its code and its comment. A # inside a string +// opens no comment, which is the whole reason this is not a call to strings.Cut. +func splitComment(line string) (code, comment string) { + quote := byte(0) + for i := 0; i < len(line); i++ { + c := line[i] + if quote != 0 { + if quote == '"' && c == '\\' { + i++ + continue + } + if c == quote { + quote = 0 + } + continue + } + switch c { + case '"', '\'': + quote = c + case '#': + return line[:i], line[i:] + } + } + return line, "" +} + +// scanCode walks the code of a line, collecting the strings in it and following +// the bracket depth. closeAt is the offset just past the bracket that brought +// the depth back to zero, or -1 while the array is still open. +func scanCode(code string, depth int) (values []string, depthOut, closeAt int) { + closeAt = -1 + for i := 0; i < len(code); i++ { + switch c := code[i]; c { + case '"', '\'': + var b strings.Builder + j := i + 1 + for j < len(code) { + if c == '"' && code[j] == '\\' && j+1 < len(code) { + b.WriteString(code[j : j+2]) + j += 2 + continue + } + if code[j] == c { + break + } + b.WriteByte(code[j]) + j++ + } + text := b.String() + if c == '"' { + text = unescape(text) + } + values = append(values, text) + i = j + case '[': + depth++ + case ']': + depth-- + if depth == 0 && closeAt < 0 { + closeAt = i + 1 } } - return i, i, true } - return 0, 0, false -} - -var stringRe = regexp.MustCompile(`"((?:[^"\\]|\\.)*)"`) - -// arrayValues pulls the strings out of an array. The array holds names — of -// topics, of languages, of stacks — and a name is a string; anything else in -// there is not a thing this tool wrote. -func arrayValues(lines []string) []string { - text := strings.Join(lines, " ") - if i := strings.Index(text, "["); i >= 0 { - text = text[i:] - } - var out []string - for _, m := range stringRe.FindAllStringSubmatch(text, -1) { - out = append(out, unescape(m[1])) - } - return out -} - -// renderArray writes the array back in the shape it had. -func renderArray(indent, key string, values []string, column bool) []string { - quoted := make([]string, len(values)) - for i, v := range values { - quoted[i] = `"` + escape(v) + `"` - } - if !column { - return []string{fmt.Sprintf("%s%s = [%s]", indent, key, strings.Join(quoted, ", "))} - } - out := []string{fmt.Sprintf("%s%s = [", indent, key)} - for _, q := range quoted { - out = append(out, indent+" "+q+",") - } - return append(out, indent+"]") + return values, depth, closeAt } // splice replaces lines from..to inclusive with the given block. diff --git a/internal/manifest/editlist_test.go b/internal/manifest/editlist_test.go index 468b544..5dcf5c0 100644 --- a/internal/manifest/editlist_test.go +++ b/internal/manifest/editlist_test.go @@ -1,9 +1,12 @@ package manifest_test import ( + "slices" "strings" "testing" + "github.com/BurntSushi/toml" + "git.vakhrushev.me/av/convy/internal/manifest" ) @@ -96,3 +99,96 @@ func TestAddTableGoesLast(t *testing.T) { t.Errorf("a second table of the same name went through") } } + +// The array is read the way the manifest wrote it. A # inside a string opens no +// comment, a bracket inside a comment closes no array, and a name inside a +// comment is not a subscription — turning any of the three into data is the one +// mistake there is no way back from. +func TestAddToListReadsCommentsAsComments(t *testing.T) { + cases := []struct { + name string + src string + add string + values []string + remains string + }{{ + name: "a name quoted inside a comment", + src: "[c.app]\ntopics = [\n \"errors\",\n # \"config\" is not taken yet\n]\n", + add: "time", + values: []string{"errors", "time"}, + remains: `# "config" is not taken yet`, + }, { + name: "a bracket inside a comment", + src: "[c.app]\ntopics = [\n \"errors\", # [enough for now]\n \"db-schema\",\n]\n", + add: "config", + values: []string{"errors", "db-schema", "config"}, + remains: `"errors", # [enough for now]`, + }, { + name: "a comment trailing a one-line array", + src: "[c.app]\ntopics = [\"git\"] # only git for now\n", + add: "time", + values: []string{"git", "time"}, + remains: `topics = ["git", "time"] # only git for now`, + }, { + name: "a hash inside a value", + src: "[c.app]\ntopics = [\"a#b\"]\n", + add: "time", + values: []string{"a#b", "time"}, + remains: `topics = ["a#b", "time"]`, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + out, err := manifest.AddToList([]byte(tc.src), "c.app", "topics", tc.add) + if err != nil { + t.Fatal(err) + } + got := decodeTopics(t, out) + if !slices.Equal(got, tc.values) { + t.Errorf("the array holds %q, expected %q:\n%s", got, tc.values, out) + } + if !strings.Contains(string(out), tc.remains) { + t.Errorf("what the author wrote is gone — no %q:\n%s", tc.remains, out) + } + }) + } +} + +// decodeTopics reads the array back with the parser the tool itself uses. +func decodeTopics(t *testing.T, source []byte) []string { + t.Helper() + var got struct { + C map[string]struct{ Topics []string } `toml:"c"` + } + if _, err := toml.Decode(string(source), &got); err != nil { + t.Fatalf("the manifest the tool wrote does not parse: %v\n%s", err, source) + } + return got.C["app"].Topics +} + +// A key is not always one line. Appending after the first line of an array +// written down a column would land the new entry inside it. +func TestAddToListAppendsPastAMultilineNeighbour(t *testing.T) { + src := "[c.app]\ndir = \"docs\"\nstack = [\n \"sqlite\",\n \"postgres\",\n]\n" + out, err := manifest.AddToList([]byte(src), "c.app", "topics", "errors") + if err != nil { + t.Fatal(err) + } + body := string(out) + if !strings.Contains(body, " \"postgres\",\n]\ntopics = [\"errors\"]") { + t.Errorf("the new key did not land past the array:\n%s", body) + } +} + +// Whatever the tool writes into a manifest, it has to be able to read back. +func TestQuoteSurvivesTheRoundTrip(t *testing.T) { + for _, value := range []string{`docs\conventions`, `a "quoted" name`, `both\ "kinds"`} { + out, err := manifest.AddToList([]byte("[c.app]\ntopics = []\n"), "c.app", "topics", value) + if err != nil { + t.Fatal(err) + } + if list := decodeTopics(t, out); len(list) != 1 || list[0] != value { + t.Errorf("%q came back as %q", value, list) + } + } +} diff --git a/internal/project/project.go b/internal/project/project.go index 6adbcf4..cb8e60e 100644 --- a/internal/project/project.go +++ b/internal/project/project.go @@ -24,10 +24,10 @@ import ( "git.vakhrushev.me/av/convy/internal/suite" ) -// LocalMarker is the one piece of markup inside a copy that means something to -// the tool. It is single and nameless, so there is no name to be orphaned by a -// rename. -const LocalMarker = "" +// LocalMarker is the boundary between what the suite wrote and what the +// repository wrote. It is defined once, next to the parsing that has to respect +// it, and named again here because assembly is where it is placed. +const LocalMarker = doc.LocalMarker // Copy is the outcome of assembling one topic for one component. type Copy struct { @@ -68,6 +68,7 @@ func Assemble(s *suite.Suite, root string, c manifest.Component, topic string) ( made.Layers = append(made.Layers, d.Path) } + local := LocalMarker + "\n" existing, err := os.ReadFile(name) switch { case os.IsNotExist(err): @@ -75,12 +76,12 @@ func Assemble(s *suite.Suite, root string, c manifest.Component, topic string) ( case err != nil: return Copy{}, err default: - if err := ours(rel, string(existing), topic); err != nil { + kept, err := ours(rel, string(existing), topic) + if err != nil { return Copy{}, err } + local = kept } - - local := localPart(string(existing)) made.Kept = strings.TrimSpace(strings.TrimPrefix(local, LocalMarker)) != "" body := Render(topic, layers, s.Vocab) + "\n\n" + local @@ -123,37 +124,34 @@ func Reading(langRoot, path, root, dir string) (string, error) { return rel, nil } -// ours refuses to overwrite a file that is not a copy of this topic. A file -// whose origin key was taken away has stopped being a copy and become a -// document of the repository, and the tool has no business rewriting it. -func ours(rel, existing, topic string) error { +// ours decides whether a file standing in the way may be rewritten, and returns +// the part of it that survives: everything from the marker down. +// +// Three ways it may not. A file whose origin key was taken away has stopped +// being a copy and become a document of the repository. A file carrying the +// origin of another topic is another copy that would be buried by this one. And +// a file with no marker cannot be rewritten either — the marker is always +// placed by the assembler, so a copy without one was edited by hand, and +// everything in it counts as suite text that assembly would silently replace. +func ours(rel, existing, topic string) (string, error) { d, err := doc.Parse(rel, existing) if err != nil { - return fmt.Errorf("%s is in the way and cannot be read: %w", rel, err) + return "", fmt.Errorf("%s is in the way and cannot be read: %w", rel, err) } switch { case d.Front.Origin == "" && d.Front.Topic == "" && d.Front.Prefix == "": - return fmt.Errorf("%s carries no origin key: it is a document of the repository rather than a copy, and assembly would overwrite it", rel) + return "", fmt.Errorf("%s carries no origin key: it is a document of the repository rather than a copy, and assembly would overwrite it", rel) case d.Front.Origin == "": - return fmt.Errorf("%s carries no origin key, while it does carry the front matter of a suite file: it looks like a layer put here by hand", rel) + return "", fmt.Errorf("%s carries no origin key, while it does carry the front matter of a suite file: it looks like a layer put here by hand", rel) case d.Front.Origin != topic: - return fmt.Errorf("%s is a copy of the topic %q, and the topic %q would be assembled into the same file", rel, d.Front.Origin, topic) + return "", fmt.Errorf("%s is a copy of the topic %q, and the topic %q would be assembled into the same file", rel, d.Front.Origin, topic) } - return nil -} -// localPart returns everything from the marker down, together with the marker. -// A file that has none gets one: the marker is placed by the assembler, and a -// copy without it has nowhere to put a derogation. -func localPart(existing string) string { - lines := strings.Split(existing, "\n") - for i, line := range lines { - if strings.TrimSpace(line) != LocalMarker { - continue - } - return strings.TrimRight(strings.Join(lines[i:], "\n"), "\n") + "\n" + marker := d.Marker() + if marker == 0 { + return "", fmt.Errorf("%s carries no %s marker, and the assembler always leaves one: whatever is in the file was written above the boundary and would be replaced without trace", rel, LocalMarker) } - return LocalMarker + "\n" + return strings.TrimRight(d.Below(marker), "\n") + "\n", nil } // Render lays out the part of a copy that comes from the suite: the front