Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pipeline/4-holders-declare-paths #652 +/- ##
===================================================================
Coverage 99.65% 99.66%
- Complexity 1917 1948 +31
===================================================================
Files 140 144 +4
Lines 4923 5004 +81
===================================================================
+ Hits 4906 4987 +81
Misses 17 17 ☔ View full report in Codecov by Harness. |
Firehed
left a comment
There was a problem hiding this comment.
The overall mechanics on this feel suspicious, even if they may work today. I haven't closely reviewed the main PollingFileWatcher yet which needs direct focus; that aside, the "stamping" utilities seem too spread out and poorly encapsulated.
|
|
||
| ## Changes on Disk | ||
|
|
||
| ALE's own LSP client does not send `workspace/didChangeWatchedFiles`. |
There was a problem hiding this comment.
Claim should specify a version/commit/etc since ALE may fix this in the future.
| // support; the events invalidate cached workspace state (RFC 1 §5.2, §5.3). | ||
| $watchedFilesRegistrar = new WatchedFilesRegistrar(new TransportClientConnection($transport)); | ||
| $lifecycleHandler = new LifecycleHandler($negotiator, [$watchedFilesRegistrar]); | ||
| $fileWatcher = new PollingFileWatcher( |
There was a problem hiding this comment.
This setup should happen in the DI container if possible. May defer if it turns into a wiring nightmare.
| private readonly LifecycleHandler $lifecycleHandler, | ||
| array $handlers, | ||
| private readonly MessageScopedInterface $messageScope, | ||
| private readonly ?BeforeMessageInterface $beforeMessage = null, |
There was a problem hiding this comment.
this being optional seems like a recipe for bugs.
|
|
||
| final readonly class PathStamp | ||
| { | ||
| public function __construct( |
There was a problem hiding this comment.
I thought the initial design had agreed on using a cheap hash rather than just time and size.
ad05b95 to
9e9a68c
Compare
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
9e9a68c to
a0145d3
Compare
Last of five stacked PRs, on top of the watched paths. Its tree is identical to #646, the total view.
What was wrong
[LSP] makes
workspace/didChangeWatchedFilesthe way a server learns of changes on disk, and leaves supporting it optional for the client. With a client that does not, nothing told the server a file changed. ALE is one: it declares no such capability and never sends the notification.What this does
PollingFileWatcherstands in for the notification with a client that did not declare support, and does nothing otherwise.InvalidatableInterfacethe notification's handler calls.StatReaderreads modification times.BeforeMessageInterfaceis how the server loop calls the watcher.Behaviour changes
With a client that lacks watched-file support, a file created, deleted, or regenerated on disk is seen on the next request.
Two things that are deliberate
Measured
#647 records the option to throttle it.
Enforcement edits
Each at Eric's direction, classified by
docs/architecture/enforcement-edits.md.phpstan.neonfilesystem allowlist: addStatReaderphpstan.neoninvalidate()allowlist: addPollingFileWatcherdeptrac.yaml: add theMessagelayerdeptrac.yaml:Rootmay useFilesystem,Message, andWatch;Watchmay useCache,Capability,Domain,Filesystem, andMessage🤖 Generated with Claude Code