Skip to content

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

Description

@willkg

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 Remove markfluence fix #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

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

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    Priority

    None yet

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions