Separate editor state (OpenFile) from analysis state (File)#1274
Conversation
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
A few important notes, but seems nice
| /// Free functions over `LineIndex` + `PositionEncoding`, so anything holding | ||
| /// those two (a `File` plus its `db`, or a `DocumentContext`) can convert | ||
| /// without each carrying its own copy of the logic. | ||
| pub(crate) fn lsp_position_from_tree_sitter_point( |
There was a problem hiding this comment.
Yea, just having free functions feels like the right way to do this to me
| pub(crate) fn code_actions( | ||
| db: &dyn ArkDb, | ||
| file: &ArkFile, | ||
| file: &OpenFile, |
There was a problem hiding this comment.
I guess if we wanted to make this File then it would be pretty complicated
We'd need to return a CodeActionResponse-ish type that we own that doesn't need file.wire_url or file.version but encodes everything else, then at the boundary we'd add in that extra info from the OpenFile
There was a problem hiding this comment.
yep that would be the cleaner way
| pub(crate) struct OpenFile { | ||
| pub(crate) inner: File, | ||
| pub(crate) version: Option<i32>, | ||
| pub(crate) config: DocumentConfig, | ||
| pub(crate) wire_url: Url, |
There was a problem hiding this comment.
Mild preference for not making the fields pub, and instead providing getters for all of them. Most importantly you'd only provide setters for version and config, making it clear that when you call open_file_mut(), those are the only 2 things you are allowed to change.
There was a problem hiding this comment.
Most importantly you'd only provide setters for version and config, making it clear that when you call open_file_mut(), those are the only 2 things you are allowed to change.
That's mildly better, doing that
| } | ||
|
|
||
| /// The salsa [`File`] handle for an open document. | ||
| pub(crate) fn file(&self, uri: &Url) -> anyhow::Result<File> { |
There was a problem hiding this comment.
Feels a bit duplicative to me over something like open_file(uri).file()
There was a problem hiding this comment.
ok, which leads to returning a & from open_file(), which is actually the right thing to do now that it's the long-term API. (We returned an owned value for ArkFile so they can cross r_task())
|
|
||
| let indent_size = position.column; | ||
| let documentation = documentation_builder(parameter_names, indent_size); | ||
|
|
There was a problem hiding this comment.
I think we lost an important We insert the documentation string comment
|
|
||
| roxygen_documentation( | ||
| let mut actions = code_actions( |
There was a problem hiding this comment.
You can't remove this. This literally doesn't even test the roxygen_documentation() function at all anymore 😢
There was a problem hiding this comment.
I think this dispatches to the roxygen code action right?
lionel-
force-pushed
the
oak/salsa-30-lazy-package-metadata
branch
from
bc0615d to
42ad025
Compare
lionel-
force-pushed
the
oak/salsa-31-open-file
branch
from
bdbc03d to
30cadc4
Compare
2 participants
Branched from #1271
Progress towards #1212
We were closer than I thought to a cleaner LSP layer file type, so I went ahead and cleaned that up.
ArkFiletoOpenFile. Now shrunk to editor/transport state (inner: File,version,config,wire_url), dropped the per-fileencodingfield which can be accessed viaLspConfig.Fileinstead of&ArkFile. Encoding is read fromstate.configat the handler boundary.state.file(uri)for handlers needing only theFile; keepstate.open_file(uri)for the two that needwire_url/config(code action, indent)indent_editandroxygen_documentationnow return domain types in tree-sitter coordinates (IndentEdit,RoxygenEdit). LSP conversion happens in the handler.