refactor: extract out a biome_line_index crate
#6222
Conversation
| macro_rules! check_conversion { | ||
| ($line_index:ident : $wide_line_col:expr => $text_size:expr ) => { | ||
| let encoding = WideEncoding::Utf16; | ||
|
|
||
| let line_col = $line_index.to_utf8(encoding, $wide_line_col); | ||
| let offset = $line_index.offset(line_col); | ||
| assert_eq!(offset, Some($text_size)); | ||
|
|
||
| let line_col = $line_index.line_col(offset.unwrap()); | ||
| let wide_line_col = $line_index.to_wide(encoding, line_col.unwrap()); | ||
| assert_eq!(wide_line_col, Some($wide_line_col)); |
There was a problem hiding this comment.
This used to use PositionEncoding::Wide(WideEncoding::Utf16), offset(), and position(), but we don't have access to those here, so I've had to tweak the tests to have the same effect. I think they are spiritually the same, but test at a slightly lower level now.
The original tests with the LSP types still exist in biome_lsp_converters
| #[test] | ||
| fn out_of_bounds_line() { | ||
| let line_index = LineIndex::new("abcde\nfghij\n"); | ||
|
|
||
| let offset = line_index.offset(LineCol { line: 5, col: 0 }); | ||
| assert!(offset.is_none()); | ||
| } |
There was a problem hiding this comment.
This lives in biome_line_index because it specifically tests LineIndex, not an LSP related thing
| #[ignore] | ||
| #[test] | ||
| fn test_every_chars() { | ||
| let text: String = { |
There was a problem hiding this comment.
This lives in biome_line_index because it specifically tests LineIndex, not an LSP related thing
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] | ||
| pub enum WideEncoding { |
There was a problem hiding this comment.
Note how you no longer have
biome_lsp_converters::WideEncoding
biome_lsp_converters::line_index::LineIndexinstead you have
biome_line_index::WideEncoding
biome_line_index::LineIndexI'm unsure of how strict you all are about breaking changes like this one
There was a problem hiding this comment.
We don't guarantee SemVer of our crates because we're a moving target, so I think it's fine to change things like this :)
DavisVaughan
changed the title
biome_line_index cratebiome_line_index crate
DavisVaughan
changed the title
biome_line_index cratebiome_line_index crate
CodSpeed Performance ReportMerging #6222 will not alter performanceComparing Summary
|
arendjr
left a comment
arendjr
left a comment
There was a problem hiding this comment.
Thanks for the PR and the kind words!
This looks totally fine for us, but you'll have to wait for the merge a bit. We're about to release 2.0, so we trying to minimise the impact right now. We'll open main again soon enough 🤞
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Thank you @DavisVaughan, I think we can merge this PR whenever you want. @arendjr what do you think? There aren't any minor changes inside @biomejs/biome, and we don't usually guarantee semver of our crates
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] | ||
| pub enum WideEncoding { |
There was a problem hiding this comment.
We don't guarantee SemVer of our crates because we're a moving target, so I think it's fine to change things like this :)
|
Thank you @ematipico and @arendjr! |
3 participants
Summary
Hiya Biome team,
We used the Biome formatter infrastructure to create Air, an R formatter. The Biome infra has been fantastic for us, so thank you all so much for your great work!
We have our own LSP server, and we want to move it from using tower-lsp -> lsp-server, and from
tower_lsp::lsp_types::*->lsp_types::*(i.e. using lsp-types directly, not reexported by tower-lsp). We tried doing this, but got roadblocked by our usage ofbiome-lsp-converters.biome-lsp-converters provides two things:
LineIndextype, which is extremely useful for conversion between byte offset and line/column position (the implementation of this is just 🔥)tower_lsp::lsp_types::*), like:from_proto::offset()from_proto::text_range()to_proto::position()to_proto::range()We really want to be able to use your great
LineIndextype for conversions, but we don't want the tower-lsp (and tokio) dependency that comes with that. In biome-lsp-converters, you import tower-lsp just to be able to access theirtower_lsp::lsp_types::*reexports, but Rust doesn't consider those reexports compatible withlsp_types::*itself.Would you consider this PR that splits out the
LineIndextype into its ownbiome_line_indexcrate? It would be nice for us to be able to depend on this lighter weight crate instead, andLineIndexseems like an extremely handy little utility for language servers in particular, so other people may also find it useful (we will probably also use it in Ark, which is more of a full blown R language server and a jupyter kernel).I'm already using this
biome_line_indexcrate here as a POC, and it worked as expected posit-dev/air#354I'm not sure if you would want
biome_lsp_convertersto reexport everything frombiome_line_indexor not. I did not do that for now, and instead updated all theusedeclarations in yourbiome_lspcrate. Let me know if that's not what you want (for example, if you don't want a breaking change in thebiome_lsp_convertersAPI).I have not changed any of the actual implementation of
LineIndex, the only things that have changed are:biome_rowandep was swapped forbiome_text_size, because it is slimmerbiome_line_indexare the old tests, but now useWideLineColinstead oflsp_types::Positionbiome_lsp_converterswere mostly kept, but I removed ones that were explicitly about just theLineIndexdetails, since they are duplicated inbiome_line_indexTest Plan
Existing tests were maintained