Rearrange to group proto/ together, and remove rust_analyzer/
#363
Conversation
| #[derive(Debug, Clone)] | ||
| pub struct LineIndex { | ||
| pub index: Arc<biome_line_index::LineIndex>, | ||
| pub endings: LineEnding, | ||
| pub encoding: PositionEncoding, | ||
| } |
There was a problem hiding this comment.
Because each conversion function now takes the biome_line_index::LineIndex, it doesn't make sense to have a wrapper that holds these other fields too.
Instead the Document itself is in charge of holding these fields.
I think this makes a lot of sense, because we throw away and rebuild the LineIndex regularly, but the endings and encoding of the Document never changes and never needs to be updated.
This also becomes apparent in my next PR to optimize on_did_change() a bit.
| Ok(biome_text_size::TextRange::new(start, end)) | ||
| } | ||
|
|
||
| pub fn apply_text_edits( |
There was a problem hiding this comment.
Only actually used by the client during testing, so it moved there.
| let line_col = line_index | ||
| .index | ||
| .line_col(offset) | ||
| .with_context(|| format!("Could not convert offset {offset:?} into a line-column index"))?; | ||
|
|
||
| let position = match position_encoding { | ||
| PositionEncoding::Utf8 => lsp_types::Position::new(line_col.line, line_col.col), | ||
| PositionEncoding::Wide(enc) => { | ||
| let line_col = line_index | ||
| .index |
There was a problem hiding this comment.
This also lets us avoid the double Arc<> access if we do decide to keep it
| pub struct LineIndex { | ||
| pub index: Arc<biome_line_index::LineIndex>, |
There was a problem hiding this comment.
I'd like to argue that we should drop this wrapper entirely at this point. We currently have no reason to wrap this in an Arc. I know rust-analyzer does this, but I think we should simplify things until we start to use salsa, and let the implementation built around salsa guide whether or not we should arc this.
There was a problem hiding this comment.
It's very refreshing to look at this file and see position -> range -> text_edit -> text_edit_vec in order as you read down the page
| // --- Start Posit | ||
| pub fn diff(text: &str, replace_with: &str) -> TextEdit { | ||
| super::diff::diff(text, replace_with) | ||
| } | ||
| // --- End Posit |
There was a problem hiding this comment.
This is less important, but we can keep text_edit.rs a "pure" vendored file if we just reference crate::diff::diff() rather than trying for TextEdit::diff(). It doesn't matter too much either way i guess, this just felt better?
Extracted from #353
The main goal of this PR is to reduce some confusion I keep having every time I look at our
to_proto.rsandfrom_proto.rsfiles. The big confusion points are:LineIndexwrapper, others takebiome_line_index::LineIndexdirectly. This was highly confusing when you're just looking at the APIs of these files, and it made the actual call sites messy sometimes. My solution is to make everything take abiome_line_index::LineIndex. Because that's the lower level helper, that means that each conversion function also explicitly takesposition_encodingand, if it needs it,endings. I think this is quite nice, as it really makes explicit what is required to do the conversion.rust_analyzer/utils felt very far away from everything else. In particular,rust_analyzer/to_proto.rsandto_proto.rsshould really be the same file, but we had this weird setup where you'd have to look back and forth between the two to get the whole picture.I've flattened out
rust_analyzer/, putting things in the following places:rust_analyzer/diff.rsis now justdiff.rsrust_analyzer/line_index.rsis now justline_index.rsrust_analyzer/utils.rswhich just heldapply_document_changes()is now inlined intodocument.rswhere it is actually used. And my next PR reworks it entirely, and also improves it.rust_analyzer/to_proto.rsandto_proto.rsare merged intoproto/to_proto.rs. Similarlyfrom_proto.rsisproto/from_proto.rs, and we haveproto.rswhich holds common helpers of the two.Really the hope is that you can look at
to_proto.rsandfrom_proto.rsnow and say "yea that makes sense" as you read it from top to bottom. So look at those files in particular and isolation and see if you feel the same way.