mozilla / mozilla/markfluence

Decide whether frontmatter.UpdateListField goes, now that nothing calls it

Open
#157 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
2
Forks
0
Avg merge
2h 8m
Merged PRs (30d)
49

Description

frontmatter.UpdateListField has had no production caller since #151 removed markfluence fix. Decide whether it goes, and with it the block-style-preservation contract it is the only way to reach.

How it got here

cmd/fix/fix.go:244 was its only non-test caller — git grep UpdateListField main~ confirms — because fix was the one verb that wrote a labels: list back into a file. Nothing else writes a frontmatter sequence surgically:

  • create's writeBackFrontmatter (cmd/create/create.go:1132) writes five scalar fields through UpdateField, and persistToManifest documents that "labels is deliberately not written" — writing them would rewrite them, since labels.Set carries normalized names.
  • update writes no files at all.
  • read/export go through Render, which builds a block from scratch rather than editing one.

PR #156 flagged this and kept the function deliberately, on the grounds that internal/frontmatter owns the frontmatter dialect rather than serving its current callers, and that deleting adjacent machinery mid-removal is how a removal grows a regression. This issue is that decision, taken on its own.

What is actually dead, precisely

The review that found this described the whole sequence-writing path as dead, which overstates it. The accurate picture:

status
UpdateListField (frontmatter.go:415) dead — no production caller
existingSeqIsFlow (:467) effectively dead — still called on every UpdateField at :441, but its return value is only consumed when f.List != nil, so no scalar write can observe it
readsBackInSeqAs (:269), elementNodeFor (:239), seqNodeFor live, via Renderread/export emit labels: through pagedoc.RenderFrontmatter

So the thing genuinely lost by deleting is narrower than "sequence writing": it is the only path that writes a sequence in block style. Render always emits flow (labels: [howto, runbook], pinned by TestRenderListField and TestEmptyListIsWrittenAsFlowEmpty). UpdateListField is the sole reader of an existing sequence's IsFlowStyle, and therefore the only code that can keep a block list a block list.

That contract has a reason worth not losing by accident: a set large enough to be written as a block list is exactly the set whose flow spelling is an unreadable single line. It is pinned by TestUpdateListFieldKeepsTheStyle (frontmatter_test.go:652) and, indirectly, TestBlockListSurvivesNormalize (:682) — note the latter covers Normalize, which is live via create, so block lists surviving a reorder is not at risk either way.

The case for deleting

  • Unreferenced exported API is a maintenance claim nobody is making good on. The contract can regress and no command will notice; only internal/frontmatter's own tests will.
  • docs/confluence/labels.md had to be amended in #156 to say outright that no command exercises this any more, which is a docs smell pointing straight at the code.
  • markfluence is unreleased, so there is no external consumer and no deprecation to run.
  • It is the honest reading of the codebase's own discipline: CLAUDE.md treats the code as self-documenting, and an exported function with no caller documents a capability the tool does not have.

The case for keeping

  • internal/frontmatter is a library for the frontmatter dialect, not a helper sized to today's callers. Every rule in it was found by probing goccy, and re-deriving the block-style rule later means re-finding the same trap.
  • UpdateListField is Render's surgical counterpart. Deleting it leaves the package able to build a sequence but not edit one, which is an odd shape for a package whose whole job is reading and writing this block.
  • #154 (markfluence diff) may want it back. diff compares frontmatter values, and if anything is ever added that reconciles one, the block-style contract is immediately load-bearing again. Worth checking #154's design before deleting.

If it is deleted

  • UpdateListField and existingSeqIsFlow, plus the flow parameter threaded through valueWithComment/seqNodeFor/elementNodeFor/readsBackInSeqAs — which then always writes flow and can lose the parameter entirely. That simplification is most of the value.
  • TestUpdateListFieldKeepsTheStyle goes; TestRenderListField and TestBlockListSurvivesNormalize stay.
  • **C2** (frontmatter-is-valid-yaml`) needs re-reading rather than assumed unaffected. It says a value may be "a sequence (in either YAML style) whose every element is one" — that is about reading, which is unchanged, but the guarantee's discussion should not be left implying markfluence can write either style when it can only write flow.
  • docs/confluence/labels.md's block-style bullet should move from "no command exercises this" to describing reading only.
  • Amend _plans/040, which records the keep decision and its reasoning.

Related

  • #151 / #156 — where the caller went, and where the keep decision was recorded.
  • #154 — check whether diff wants a metadata write path before deleting.
  • One smaller item of the same kind, worth resolving in the same pass: internal/labels's exported Field const (labels.go:42) now has no reference outside its own package. It is used at three internal sites so it is not dead, but it is no longer part of anyone's API and could be unexported.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by checking #154's design, then read internal/frontmatter/frontmatter.go around UpdateListField and existingSeqIsFlow. Run the frontmatter tests, especially TestUpdateListFieldKeepsTheStyle, TestRenderListField, and TestBlockListSurvivesNormalize. Done means the keep-or-delete decision is implemented consistently in the code, tests, docs/confluence/labels.md, C2, and _plans/040.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
cli
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.