Skip to content

Consolidate the three notice joiners, so the one-notice-per-line contract is written down once #390

Description

@thecodedrift

Three places build an optional notice from a list of strings, in two different shapes:

packages/cli/src/rules/dispatch.ts:262
  function joinNotices(notices: Array<string | undefined>): string | undefined {
    const present = notices.filter((notice) => notice !== undefined);
    return present.length === 0 ? undefined : present.join("\n");
  }

packages/cli/src/rules/inspect.ts:290
  ...(advisories.length === 0 ? {} : { notice: advisories.join("\n") }),

packages/cli/src/rules/vale/run.ts:401
  const notice =
    advisories.length === 0 ? {} : { notice: advisories.join("\n") };

The duplication is small. That is not the reason to fix it.

The separator is a contract, and it is written down nowhere

"\n" is load-bearing. packages/cli/src/commands/verify.ts prefixes EVERY line of a multi-line notice with its own notice: marker — that was the fix in 241e1c4, after a multi-line notice rendered with only its first line marked. So "one notice per line" is an agreement between three producers and one renderer, and today it exists only as three identical string literals that nobody is obliged to keep identical.

A producer that joined with "; " or a blank line would not fail any test. It would quietly mis-render, in the direction that is hardest to notice: the notice still appears, just indented wrongly.

What to do

  • One function, in a leaf module with no imports of its own — packages/cli/src/util/notices.ts is the obvious home. Return string | undefined.
  • A leaf module specifically. dispatch.ts importing a helper out of vale/run.ts (or the reverse) would add an edge inside src/rules/ between modules that already sit close to a cycle. See the filesystem/migrate.ts cycle fixed in feat(verify): refuse a rule id held by more than one engine #388, which cost real time and was found by luck.
  • Keep the ...(x === undefined ? {} : { notice: x }) spread at the call sites. Do not build a helper that returns an object-or-empty to save the spread; the plain form is readable and does not hide the optionality from exactOptionalPropertyTypes.
  • Put the contract in the function's docblock: one notice per line, undefined when there are none, and the reason — verify prefixes each line, so the separator is not a formatting preference.
  • Add the test that is actually missing: a multi-line notice reaching verify output with every line prefixed. That is the assertion that would catch a separator change, and no amount of single-file refactoring substitutes for it.

Timing

Do this after #385 and #388 land. All three files are under packages/cli/src/rules/, both of those PRs touch that tree, and this is pure cleanup with no deadline. Not worth a rebase conflict.

Refs #388

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

    CLIRelated to the taskless CLI

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions