fix: Empty fields required #129
Conversation
WalkthroughThis update refines the handling of optional and mandatory fields within code snippets across multiple programming languages, enhancing the flexibility and accuracy of code-snippet matching. It introduces a new approach to manage optional empty fields in several language modules and simplifies the handling of unused imports in test configurations, aiming to improve the overall codebase integrity and snippet matching capabilities. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (8)
- crates/core/src/pattern/patterns.rs (1 hunks)
- crates/core/src/test.rs (3 hunks)
- crates/language/src/javascript.rs (4 hunks)
- crates/language/src/json.rs (3 hunks)
- crates/language/src/language.rs (1 hunks)
- crates/language/src/rust.rs (3 hunks)
- crates/language/src/tsx.rs (5 hunks)
- crates/language/src/typescript.rs (5 hunks)
Additional Context Used
Additional comments not posted (14)
crates/language/src/json.rs (1)
3-3: The removal ofMANDATORY_EMPTY_FIELD_SORTSsimplifies theJsonstruct. Ensure that the handling of mandatory fields is correctly managed elsewhere or that this concept is no longer needed.crates/language/src/rust.rs (1)
9-9: The introduction ofOPTIONAL_EMPTY_FIELD_COMPILATIONis a significant addition to handle optional empty fields in Rust language structures. Ensure that the initialization and usage are correctly implemented.crates/language/src/javascript.rs (1)
17-17: The addition ofOPTIONAL_EMPTY_FIELD_COMPILATIONfor handling optional empty fields in JavaScript structures is a welcome improvement. Ensure its initialization and usage align with JavaScript's dynamic nature.crates/language/src/typescript.rs (1)
15-15: The introduction ofOPTIONAL_EMPTY_FIELD_COMPILATIONin TypeScript structures is an important step towards improving the handling of optional empty fields. Ensure its implementation is consistent with TypeScript's static typing system.crates/language/src/language.rs (1)
163-163: Renamingmandatory_empty_fieldtooptional_empty_field_compilationin theLanguagetrait aligns with the new focus on handling optional empty fields. Ensure this change is consistently applied across all language implementations.crates/language/src/tsx.rs (4)
16-16: The addition of theOPTIONAL_EMPTY_FIELD_COMPILATIONstatic variable is a key part of the changes. It's crucial to ensure that this variable is correctly initialized and used throughout the file.
37-37: Theoptional_empty_field_compilationfield has been added to theTsxstruct. This is a significant change as it directly impacts how theTsxstruct handles optional empty fields during compilation. It's important to ensure that this field is properly initialized and used.
115-170: The initialization of theOPTIONAL_EMPTY_FIELD_COMPILATIONvariable involves specifying a list of(SortId, FieldId)tuples. This list determines which fields are considered optional and empty during the compilation process. It's crucial to verify that the specified fields align with the intended behavior for handling optional empty fields in TSX code snippets.
194-202: The implementation of theoptional_empty_field_compilationmethod in theTsxstruct is a critical part of the changes. This method checks if a given(SortId, FieldId)tuple is present in theoptional_empty_field_compilationlist, affecting how optional empty fields are compiled. It's important to ensure that this method functions correctly to achieve the desired behavior.crates/core/src/pattern/patterns.rs (1)
509-588: The logic for handling optional and mandatory fields during the compilation of AST nodes has been refined. This includes filtering fields based on whether they are considered optional empty fields or if their compilation should be skipped. It's crucial to ensure that this logic accurately reflects the intended behavior for matching fields in code snippets, especially in the context of TSX code where certain fields may be optional.crates/core/src/test.rs (4)
5359-5359: The pattern matching logic introduced here appears correct. Ensure that the$name($_)syntax and the$fns += $fnoperation are consistent with and supported by the rest of the codebase.
5407-5413: The extension of pattern matching logic, including the joining of matched functions and the condition for a specific function name, is correctly implemented. Verify the correct use and support for thejoinandcontains bubbleoperations within the codebase.
12960-12984: Thedelete_import_clausetest function correctly implements the pattern for matching and removing import statements. Verify the correct use and support for the pattern syntax and thetrim_margin().unwrap()method calls within the codebase.
12986-13032: Theremove_unused_importtest function introduces complex pattern matching for identifying and removing unused imports. Ensure that the pattern syntax, conditional logic, and variable usage are correctly implemented and supported within the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- crates/core/src/pattern/patterns.rs (2 hunks)
- crates/core/src/test.rs (3 hunks)
- crates/language/src/language.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- crates/core/src/pattern/patterns.rs
- crates/core/src/test.rs
- crates/language/src/language.rs
Additional Context Used
|
On commit fc7e4ba, Grit tried to heal these tests:
Tip You can view and edit CI healing settings on the Grit App. |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/core/src/pattern/patterns.rs (2 hunks)
Additional Context Used
GitHub Check Runs (1)
clippy failure (1)
crates/core/src/pattern/patterns.rs: [failure] 520-539: this boolean expression can be simplified
error: this boolean expression can be simplified
--> crates/core/src/pattern/patterns.rs:520:25
|
520 | / !(node.child_by_field_id(field.id()).is_none() && lang.optional_empty_field_compilation(sort, field.id()))
521 | | // we wanted to be able to match on the presence of parentheses in an arrow function manually
522 | | // using ast_node syntax, but we wanted snippets to match regardless of weather or not the
523 | | // parenthesis are present, so we made the parenthesis a named node within a field, but
... |
538 | | // }
539 | | && !lang.skip_snippet_compilation_of_field(sort, field.id())
| |____________________________________________________________________________________^ help: try:!(lang.skip_snippet_compilation_of_field(sort, field.id()) || node.child_by_field_id(field.id()).is_none() && lang.optional_empty_field_compilation(sort, field.id()))
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
= note:-D clippy::nonminimal-boolimplied by-D warnings
= help: to override-D warningsadd#[allow(clippy::nonminimal_bool)]
crates/core/src/pattern/patterns.rs
Outdated
| let field_id = field.id(); | ||
| let mut cursor = node.walk(); | ||
| let mut nodes_list = node | ||
| .children_by_field_id(field_id, &mut cursor) | ||
| .filter(|n| n.is_named()) | ||
| .map(|n| { | ||
| node_to_astnode( | ||
| n, | ||
| context_range, | ||
| file, | ||
| text, | ||
| lang, | ||
| range_map, | ||
| vars, | ||
| global_vars, | ||
| vars_array, | ||
| scope_index, | ||
| is_rhs, | ||
| ) | ||
| }) | ||
| .collect::<Result<Vec<Pattern>>>()?; | ||
| if !field.multiple() { | ||
| return Ok(( | ||
| field_id, | ||
| false, | ||
| nodes_list.pop().unwrap_or(Pattern::Dynamic( | ||
| DynamicPattern::Snippet(DynamicSnippet { | ||
| parts: vec![DynamicSnippetPart::String("".to_string())], | ||
| }), | ||
| )), | ||
| )); | ||
| } | ||
| if nodes_list.len() == 1 | ||
| && matches!( | ||
| nodes_list.first(), | ||
| Some(Pattern::Variable(_)) | Some(Pattern::Underscore) | ||
| ) | ||
| }) | ||
| .collect::<Result<Vec<Pattern>>>()?; | ||
|
|
||
| // TODO check if Pattern is Dots, and error at compile time, | ||
| // dots only makes sense in a list. | ||
| if !field.multiple() { | ||
| if nodes_list.len() == 1 { | ||
| return Ok((field_id, false, nodes_list.pop().unwrap())); | ||
| { | ||
| return Ok((field_id, true, nodes_list.pop().unwrap())); | ||
| } | ||
| let field_node = node.child_by_field_id(field_id).unwrap(); | ||
| let field_node_with_source = NodeWithSource::new(field_node.clone(), str::from_utf8(text).unwrap()); | ||
| return Ok((field_id, false, Pattern::AstLeafNode(AstLeafNode::new( | ||
| field_node.kind_id(), field_node_with_source.text(), lang, | ||
| )?))); | ||
| } | ||
| if nodes_list.len() == 1 | ||
| && matches!( | ||
| nodes_list.first(), | ||
| Some(Pattern::Variable(_)) | Some(Pattern::Underscore) | ||
| ) | ||
| { | ||
| return Ok((field_id, true, nodes_list.pop().unwrap())); | ||
| } | ||
| Ok(( | ||
| field_id, | ||
| true, | ||
| Pattern::List(Box::new(List::new(nodes_list))), | ||
| )) | ||
| }) | ||
| .collect::<Result<Vec<(u16, bool, Pattern)>>>()?; | ||
| let mut mandatory_empty_args = fields | ||
| .iter() | ||
| .filter(|field| { | ||
| node.child_by_field_id(field.id()).is_none() | ||
| && lang.mandatory_empty_field(sort, field.id()) | ||
| }) | ||
| .map(|field| { | ||
| if field.multiple() { | ||
| ( | ||
| field.id(), | ||
| Ok(( | ||
| field_id, | ||
| true, | ||
| Pattern::List(Box::new(List::new(Vec::new()))), | ||
| ) | ||
| } else { | ||
| ( | ||
| field.id(), | ||
| false, | ||
| Pattern::Dynamic(DynamicPattern::Snippet(DynamicSnippet { | ||
| parts: vec![DynamicSnippetPart::String("".to_string())], | ||
| })), | ||
| ) | ||
| } | ||
| }) | ||
| .collect::<Vec<(u16, bool, Pattern)>>(); | ||
| args.append(&mut mandatory_empty_args); | ||
| Pattern::List(Box::new(List::new(nodes_list))), | ||
| )) | ||
| }) | ||
| .collect::<Result<Vec<(u16, bool, Pattern)>>>()?; |
There was a problem hiding this comment.
Refactor the boolean expression for clarity and efficiency.
- !(node.child_by_field_id(field.id()).is_none() && lang.optional_empty_field_compilation(sort, field.id()))
- && !lang.skip_snippet_compilation_of_field(sort, field.id())
+ !(lang.skip_snippet_compilation_of_field(sort, field.id()) || node.child_by_field_id(field.id()).is_none() && lang.optional_empty_field_compilation(sort, field.id()))This change simplifies the boolean expression, making it easier to understand and potentially more efficient by reducing the number of operations.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let args = | |
| fields | |
| .iter() | |
| .filter(|field| { | |
| // ordinarily we want to match on all possible fields including the absense of nodes within a field | |
| // eg. `func_call()` should not match `func_call(arg)` however sometimes we want to allow people to | |
| // save some boilerplate and by default match a node even if a field is present in the code, but not | |
| // in the snippet. eg. | |
| // `func name(args) {}` will match `async name(args) {}` because async is an optional_empty_field for tsx. | |
| // to explicitly only match synchronous functions you could write: | |
| // `$async func name(args)` where $async <: . | |
| !(node.child_by_field_id(field.id()).is_none() && lang.optional_empty_field_compilation(sort, field.id())) | |
| // we wanted to be able to match on the presence of parentheses in an arrow function manually | |
| // using ast_node syntax, but we wanted snippets to match regardless of weather or not the | |
| // parenthesis are present, so we made the parenthesis a named node within a field, but | |
| // added then to this list so that they wont be compiled. fields in this list are | |
| // destinguished by fields in the above list in that they will NEVER prevent a match | |
| // while fields in the above list wont prevent a match if they are absent in the snippet, | |
| // but they will prevent a match if present in the snippet, and not present in the target | |
| // file. | |
| // in react to hooks we manually match the parenthesis like so: | |
| // arrow_function(parameters=$props, $body, $parenthesis) where { | |
| // $props <: contains or { `props`, `inputProps` }, | |
| // $body <: not contains `props`, | |
| // if ($parenthesis <: .) { | |
| // $props => `()` | |
| // } else { | |
| // $props => . | |
| // } | |
| // } | |
| && !lang.skip_snippet_compilation_of_field(sort, field.id()) | |
| }) | |
| .map(|field| { | |
| let field_id = field.id(); | |
| let mut cursor = node.walk(); | |
| let mut nodes_list = node | |
| .children_by_field_id(field_id, &mut cursor) | |
| .filter(|n| n.is_named()) | |
| .map(|n| { | |
| node_to_astnode( | |
| n, | |
| context_range, | |
| file, | |
| text, | |
| lang, | |
| range_map, | |
| vars, | |
| global_vars, | |
| vars_array, | |
| scope_index, | |
| is_rhs, | |
| }) | |
| .map(|field| { | |
| let field_id = field.id(); | |
| let mut cursor = node.walk(); | |
| let mut nodes_list = node | |
| .children_by_field_id(field_id, &mut cursor) | |
| .filter(|n| n.is_named()) | |
| .map(|n| { | |
| node_to_astnode( | |
| n, | |
| context_range, | |
| file, | |
| text, | |
| lang, | |
| range_map, | |
| vars, | |
| global_vars, | |
| vars_array, | |
| scope_index, | |
| is_rhs, | |
| ) | |
| }) | |
| .collect::<Result<Vec<Pattern>>>()?; | |
| if !field.multiple() { | |
| return Ok(( | |
| field_id, | |
| false, | |
| nodes_list.pop().unwrap_or(Pattern::Dynamic( | |
| DynamicPattern::Snippet(DynamicSnippet { | |
| parts: vec![DynamicSnippetPart::String("".to_string())], | |
| }), | |
| )), | |
| )); | |
| } | |
| if nodes_list.len() == 1 | |
| && matches!( | |
| nodes_list.first(), | |
| Some(Pattern::Variable(_)) | Some(Pattern::Underscore) | |
| ) | |
| }) | |
| .collect::<Result<Vec<Pattern>>>()?; | |
| // TODO check if Pattern is Dots, and error at compile time, | |
| // dots only makes sense in a list. | |
| if !field.multiple() { | |
| if nodes_list.len() == 1 { | |
| return Ok((field_id, false, nodes_list.pop().unwrap())); | |
| { | |
| return Ok((field_id, true, nodes_list.pop().unwrap())); | |
| } | |
| let field_node = node.child_by_field_id(field_id).unwrap(); | |
| let field_node_with_source = NodeWithSource::new(field_node.clone(), str::from_utf8(text).unwrap()); | |
| return Ok((field_id, false, Pattern::AstLeafNode(AstLeafNode::new( | |
| field_node.kind_id(), field_node_with_source.text(), lang, | |
| )?))); | |
| } | |
| if nodes_list.len() == 1 | |
| && matches!( | |
| nodes_list.first(), | |
| Some(Pattern::Variable(_)) | Some(Pattern::Underscore) | |
| ) | |
| { | |
| return Ok((field_id, true, nodes_list.pop().unwrap())); | |
| } | |
| Ok(( | |
| field_id, | |
| true, | |
| Pattern::List(Box::new(List::new(nodes_list))), | |
| )) | |
| }) | |
| .collect::<Result<Vec<(u16, bool, Pattern)>>>()?; | |
| let mut mandatory_empty_args = fields | |
| .iter() | |
| .filter(|field| { | |
| node.child_by_field_id(field.id()).is_none() | |
| && lang.mandatory_empty_field(sort, field.id()) | |
| }) | |
| .map(|field| { | |
| if field.multiple() { | |
| ( | |
| field.id(), | |
| Ok(( | |
| field_id, | |
| true, | |
| Pattern::List(Box::new(List::new(Vec::new()))), | |
| ) | |
| } else { | |
| ( | |
| field.id(), | |
| false, | |
| Pattern::Dynamic(DynamicPattern::Snippet(DynamicSnippet { | |
| parts: vec![DynamicSnippetPart::String("".to_string())], | |
| })), | |
| ) | |
| } | |
| }) | |
| .collect::<Vec<(u16, bool, Pattern)>>(); | |
| args.append(&mut mandatory_empty_args); | |
| Pattern::List(Box::new(List::new(nodes_list))), | |
| )) | |
| }) | |
| .collect::<Result<Vec<(u16, bool, Pattern)>>>()?; | |
| .filter(|field| { | |
| // ordinarily we want to match on all possible fields including the absense of nodes within a field | |
| // eg. `func_call()` should not match `func_call(arg)` however sometimes we want to allow people to | |
| // save some boilerplate and by default match a node even if a field is present in the code, but not | |
| // in the snippet. eg. | |
| // `func name(args) {}` will match `async name(args) {}` because async is an optional_empty_field for tsx. | |
| // to explicitly only match synchronous functions you could write: | |
| // `$async func name(args)` where $async <: . | |
| !(lang.skip_snippet_compilation_of_field(sort, field.id()) || node.child_by_field_id(field.id()).is_none() && lang.optional_empty_field_compilation(sort, field.id())) | |
| // we wanted to be able to match on the presence of parentheses in an arrow function manually | |
| // using ast_node syntax, but we wanted snippets to match regardless of weather or not the | |
| // parenthesis are present, so we made the parenthesis a named node within a field, but | |
| // added then to this list so that they wont be compiled. fields in this list are | |
| // destinguished by fields in the above list in that they will NEVER prevent a match | |
| // while fields in the above list wont prevent a match if they are absent in the snippet, | |
| // but they will prevent a match if present in the snippet, and not present in the target | |
| // file. | |
| // in react to hooks we manually match the parenthesis like so: | |
| // arrow_function(parameters=$props, $body, $parenthesis) where { | |
| // $props <: contains or { `props`, `inputProps` }, | |
| // $body <: not contains `props`, | |
| // if ($parenthesis <: .) { | |
| // $props => `()` | |
| // } else { | |
| // $props => . | |
| // } | |
| // } | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/core/src/pattern/patterns.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/core/src/pattern/patterns.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (6)
- crates/core/src/pattern/patterns.rs (2 hunks)
- crates/language/src/javascript.rs (5 hunks)
- crates/language/src/language.rs (2 hunks)
- crates/language/src/tsx.rs (5 hunks)
- crates/language/src/typescript.rs (6 hunks)
- crates/language/src/xscript_util.rs (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- crates/core/src/pattern/patterns.rs
- crates/language/src/javascript.rs
- crates/language/src/language.rs
- crates/language/src/tsx.rs
- crates/language/src/typescript.rs
Additional comments not posted (4)
crates/language/src/xscript_util.rs (4)
11-11: Consider adding a brief comment aboveSTATEMENT_NODE_NAMESexplaining its purpose and usage within the codebase.
48-56: Ensure that the pairs injs_skip_snippet_compilation_sortsare accurate and necessary for the intended logic. Double-check the field names for correctness and completeness.
76-85: Validate the field names injs_optional_empty_field_compilationto ensure they align with the language specifications and intended behavior.
88-97: Cross-verify the field names and node types injs_like_optional_empty_field_compilationfor accuracy and consistency with language specifications.
| @@ -38,13 +38,66 @@ pub static STATEMENT_NODE_NAMES: &[&str] = &[ | |||
| "with_statement", | |||
| ]; | |||
|
|
|||
| pub fn jslike_get_statement_sorts(lang: &TSLanguage) -> Vec<SortId> { | |||
| pub(crate) fn js_like_get_statement_sorts(lang: &TSLanguage) -> Vec<SortId> { | |||
There was a problem hiding this comment.
Change the visibility of js_like_get_statement_sorts from pub(crate) to pub if it needs to be accessed outside the current crate, as per the previous visibility level.
- pub(crate) fn js_like_get_statement_sorts(lang: &TSLanguage) -> Vec<SortId> {
+ pub fn js_like_get_statement_sorts(lang: &TSLanguage) -> Vec<SortId> {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub(crate) fn js_like_get_statement_sorts(lang: &TSLanguage) -> Vec<SortId> { | |
| pub fn js_like_get_statement_sorts(lang: &TSLanguage) -> Vec<SortId> { |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/language/src/xscript_util.rs (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/language/src/xscript_util.rs
fixes: #102
snippets with empty fields now only match code that also has those fields missing
Summary by CodeRabbit
New Features
Refactor
Documentation