refactor: track too large files inside workspace server #4738
Conversation
| @@ -75,7 +75,7 @@ pub(crate) struct Document { | |||
| pub(crate) file_source_index: usize, | |||
|
|
|||
| /// The result of the parser (syntax tree + diagnostics). | |||
| pub(crate) syntax: AnyParse, | |||
| pub(crate) syntax: Result<AnyParse, FileTooLarge>, | |||
There was a problem hiding this comment.
I think at this point we should start thinking about splitting WorkspaceError into two types:
WorkspaceError: actual errors that can occur during the call of a workspace API (missing method, serialisation mistakes, failing to call the server, etc.WorkspaceDiagnostics: a new type, where we collect all diagnostics during the lifecycle of an API (parsing errors, file too large, file ignored, etc.). These diagnostics aren't errors and shouldn't abruptly stop an API call, contrary toWorkspaceError
What do you think?
There was a problem hiding this comment.
Yeah agreed, that’s a good suggestion 👍 Only reason I didn’t yet is because we only have a single kind for now, but if we need to have multiple diagnostics I’ll do it like this.
| @@ -75,7 +75,7 @@ pub(crate) struct Document { | |||
| pub(crate) file_source_index: usize, | |||
|
|
|||
| /// The result of the parser (syntax tree + diagnostics). | |||
| pub(crate) syntax: AnyParse, | |||
| pub(crate) syntax: Result<AnyParse, FileTooLarge>, | |||
There was a problem hiding this comment.
Maybe the document can have a new field called diagnostics instead. This could be useful to collect more than one diagnostic related to this document (file size, ignored, unknown document, etc.
There was a problem hiding this comment.
Unlike parser errors, of which there can be multiple in a document, I think there will be only one reason (at a time) that would prevent parsing altogether. That’s why I went for a Result type, since it clearly communicates we bailed out before we even got to the parsing.
In theory we could use a Vec of diagnostics in the Err variant, and if we ever need to it’s an easy change. I’m just not convinced yet we will need to.
There was a problem hiding this comment.
On second consideration, maybe an Either could communicate the semantics even slightly clearer than a Result. After all, it wasn't an error of the parser; instead, we either parse or store a diagnostic. But it's still an error of some kind, so I wonder if this subtle semantic difference is worth the extra dependency 😅
arendjr
force-pushed
the
arendjr/track-too-large-files
branch
from
1d0ef4a to
b90cbe3
Compare
Summary
As the workspace server will become responsible for scanning projects, we also need to move the check for too large files into the server. Because there will be a disconnect between the initiation of the scanning and the pulling of diagnostics, we need to persist the
FileTooLargediagnostics. This PR implements a minimum version of this.Later, I want to implement a
pull_diagnostics_multiple()workspace method that can pull diagnostics for a scanned project as a whole. At that point, the persistedFileTooLargewill serve so we can return the right diagnostic at that point. Do you foresee any issues with this approach, @ematipico ?Test Plan
Added a test case.