lsp: three follow-ups from the #1983 reviews — one inaccurate log, one duplicated predicate, two comments claiming more than they prove
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 thegetByFilesystemPathassertion says an overtaken open "still releases" a waiter blocked on the build gate. The assertion only covers the path index. The claim is true —endBuildwakes 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 shadowedstoreand 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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