picatz / picatz/flowstate

lsp: three follow-ups from the #1983 reviews — one inaccurate log, one duplicated predicate, two comments claiming more than they prove

Open
#1,997 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
9
Forks
0
Avg merge
3h 3m
Merged PRs (30d)
509

Description

Recorded from the four review rounds on #1983 (merged as 139a667). None of these is a defect in shipped behaviour; each was explicitly judged non-blocking by the reviewer that found it. Filed so they are not lost in a transcript.

didChange logs "ignored stale change" for a change that is not stale

pkg/flowstate/v1/flowfile/lsp/server.go logs ignored stale change whenever documentStore.change returns nil. #1983 added a second reason for nil — a change set with no stored document that does not establish the whole text — so the message is now inaccurate on that path. Debug-level only, but the two cases have different remedies: a stale change means the client is behind, while an unbased change means the didOpen has not landed yet. Distinguishing them costs a sentinel or a second return value.

The ordering predicate is written twice

documentStore.open and documentStore.change each carry a version comparison with its own explanatory comment, and they deliberately differ: open uses prev.version > 0 && version <= prev.version, change keeps version > 0 && prev.version > 0 && version <= prev.version. The asymmetry is correct and reasoned — a change's version is the version after an edit and strictly increases, so an incoming zero there can only come from a client that does not track versions at all — but #1965's stated outcome is that "open and change agree about ordering", and right now that agreement is asserted by two comments rather than expressed by one mechanism.

A shared predicate taking the stored and incoming versions, with the difference passed in or named, would make the agreement structural. The two call sites diverge in what they do with the result (return nil versus keeping the previous document), so only the predicate would move. Worth doing when someone next touches either guard; not worth a change on its own.

Two comments claim slightly more than their assertions prove

  • In TestStoreRejectsAnOvertakenOpen, the comment on the getByFilesystemPath assertion says an overtaken open "still releases" a waiter blocked on the build gate. The assertion only covers the path index. The claim is true — endBuild wakes unconditionally — but a wake regression would be masked by that, so the comment describes a property the test does not pin. Either assert the wake or narrow the comment.
  • The new empty-change-set case loops over [][]lsp.TextDocumentContentChangeEvent{{}, nil} with a shadowed store and no name per shape, so a failure would not say whether the empty slice or the nil slice broke. The rest of the file names its stores distinctly (whole, mixed, untracked); this one should say which shape it is asserting.

Why these are not in the merged PR

Each was found by a reviewer on a head that was already passing, and taking them would have meant another head and another full review round for changes that alter no behaviour. The line I drew was: fix what is reachable, file what is not, and say which is which. These are the "file" side of that.

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 in pkg/flowstate/v1/flowfile/lsp/server.go, tracing documentStore.open, documentStore.change, and the didChange logging path. Then locate TestStoreRejectsAnOvertakenOpen and the empty-change-set case, and run the related Go tests. Done means the log distinguishes its nil-result causes, the ordering logic is shared without changing its asymmetry, and the comments or assertions accurately describe both test cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
testing, tooling
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.