-
-
Notifications
You must be signed in to change notification settings - Fork 511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add additional FeatureKind
entries for debug features
#3892
feat: add additional FeatureKind
entries for debug features
#3892
Conversation
e6c6f7e
to
109e6ab
Compare
CodSpeed Performance ReportMerging #3892 will improve performances by 22.95%Comparing Summary
Benchmarks breakdown
|
109e6ab
to
1792846
Compare
@@ -403,9 +419,16 @@ pub enum FeatureKind { | |||
OrganizeImports, | |||
Search, | |||
Assists, | |||
SyntaxTree, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax and formatter IR aren't needed. If a file can be handled, it can be parsed, hence it can emit a CST. If a file can be formatted, it can emit a IR.
Control flow is a black sheep. I suppose we can add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debug methods can technically be unimplemented, and that leads to throwing errors in the playground.
For example, the html file handler is currently in this state:
biome/crates/biome_service/src/file_handlers/html.rs
Lines 59 to 83 in 1792846
impl ExtensionHandler for HtmlFileHandler { | |
fn capabilities(&self) -> Capabilities { | |
Capabilities { | |
parser: ParserCapabilities { parse: Some(parse) }, | |
debug: DebugCapabilities { | |
debug_syntax_tree: None, | |
debug_control_flow: None, | |
debug_formatter_ir: None, | |
}, | |
analyzer: AnalyzerCapabilities { | |
lint: None, | |
code_actions: None, | |
rename: None, | |
fix_all: None, | |
organize_imports: None, | |
}, | |
formatter: FormatterCapabilities { | |
format: Some(format), | |
format_range: None, | |
format_on_type: None, | |
}, | |
search: SearchCapabilities { search: None }, | |
} | |
} | |
} |
I think having these will generally reduce the friction when adding new languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debug methods can technically be unimplemented
That is true for all methods, and it should be the the client to deal with that. That's a design decision because various clients, even servers, can consume the Workspace. When used by servers, all methods can technically fail, that's why even methods that don't need to return Result
, they do.
Unfortunately, JS doesn't provide any signature for when a method can throw an error at runtime, so we are stuck with what we have.
Regarding the FeatureKind
. I think we should reduce the proposed changes to a single FeatureKind::Debug
, because that's in line with what we have today. For example, we have ::Format
, not ::Formating
/::FormatOnType
/::FormatRange
. It will be up to us to implement all the debugging features at once.
007a017
to
3e1265a
Compare
3e1265a
to
cc70b3d
Compare
Summary
In the playground, we currently hard code which languages can do what with naive conditions based on the filename. This aims to make it so we don't need to constantly update the playground as file capabilities change.
This also fixes the de/serialization of the
FeatureName
type.Test Plan
CI should pass. Unsure how to test this with my local version of the playground.