-
Notifications
You must be signed in to change notification settings - Fork 211
fix: quote marks during autocomplete now take care of the existing quotes #1666
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
a2fdc89
26e462d
bc16325
44c9c09
61847f4
35e07a2
58798ba
285379f
f4541ac
b727739
3012d71
309300c
e16f11f
ea4270e
9b549c9
f04479a
c78f732
005801c
59b94ca
d0f3220
7081e24
7f4cc24
4a2fa2a
ed7a356
4d42f7b
6db8cdf
3f00984
b242eed
e5bf11e
4e02534
7c365fa
aeb0c55
32cfc95
29bfa47
7aa0e90
11c7137
1ff453d
deb5c23
394a6fa
143a3b0
7c98ce6
ad4e361
38fcb67
b2fe8ee
64a4af2
a4f50d4
1851ee7
935f9bc
65aef3d
acc00ad
f00a366
5c8d4f9
c781bfa
3e803c5
5f28775
10ac53c
13bb911
6e7d7cb
63f86cb
aa455f5
b26bb7e
ef09f28
ebadfed
08dfad0
6e4267d
02b24ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
|
|
||
| use lsp_types::CompletionItem; | ||
| use lsp_types::CompletionItemKind; | ||
| use pretty_assertions::assert_eq; | ||
| use pyrefly_build::handle::Handle; | ||
| use ruff_text_size::TextSize; | ||
|
|
||
| use crate::state::lsp::ImportFormat; | ||
| use crate::state::require::Require; | ||
| use crate::state::state::State; | ||
| use crate::test::util::get_batched_lsp_operations_report_allow_error; | ||
|
|
||
| #[derive(Default)] | ||
| struct ResultsFilter { | ||
| include_keywords: bool, | ||
| include_builtins: bool, | ||
| } | ||
|
|
||
| fn get_default_test_report() -> impl Fn(&State, &Handle, TextSize) -> String { | ||
| get_test_report(ResultsFilter::default(), ImportFormat::Absolute) | ||
| } | ||
|
|
||
| fn get_test_report( | ||
| filter: ResultsFilter, | ||
| import_format: ImportFormat, | ||
| ) -> impl Fn(&State, &Handle, TextSize) -> String { | ||
| move |state: &State, handle: &Handle, position: TextSize| { | ||
| let mut report = "Completion Results:".to_owned(); | ||
| for CompletionItem { | ||
| label, | ||
| detail, | ||
| kind, | ||
| insert_text, | ||
| data, | ||
| tags, | ||
| text_edit, | ||
| documentation, | ||
| .. | ||
| } in state | ||
| .transaction() | ||
| .completion(handle, position, import_format, true) | ||
| { | ||
| let is_deprecated = if let Some(tags) = tags { | ||
| tags.contains(&lsp_types::CompletionItemTag::DEPRECATED) | ||
| } else { | ||
| false | ||
| }; | ||
| if (filter.include_keywords || kind != Some(CompletionItemKind::KEYWORD)) | ||
| && (filter.include_builtins || data != Some(serde_json::json!("builtin"))) | ||
| { | ||
| report.push_str("\n- ("); | ||
| report.push_str(&format!("{:?}", kind.unwrap())); | ||
| report.push_str(") "); | ||
| if is_deprecated { | ||
| report.push_str("[DEPRECATED] "); | ||
| } | ||
| report.push_str(&label); | ||
| if let Some(detail) = detail { | ||
| report.push_str(": "); | ||
| report.push_str(&detail); | ||
| } | ||
| if let Some(insert_text) = insert_text { | ||
| report.push_str(" inserting `"); | ||
| report.push_str(&insert_text); | ||
| report.push('`'); | ||
| } | ||
| if let Some(text_edit) = text_edit { | ||
| report.push_str(" with text edit: "); | ||
| report.push_str(&format!("{:?}", &text_edit)); | ||
| } | ||
| if let Some(documentation) = documentation { | ||
| report.push('\n'); | ||
| match documentation { | ||
| lsp_types::Documentation::String(s) => { | ||
| report.push_str(&s); | ||
| } | ||
| lsp_types::Documentation::MarkupContent(content) => { | ||
| report.push_str(&content.value); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| report | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn completion_literal_quote_test() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of making this new test file, is there a way to include this test in the normal lsp/completion.rs test? for example,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should i write a new test in the completion.rs or i can just rewrite the completion_literal_do_not_duplicate_quotes test?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if rewriting completion_literal_do_not_duplicate_quotes is easier, go for it!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay great i will work on this rn |
||
| let code = r#" | ||
| from typing import Literal | ||
| def foo(fruit: Literal["apple", "pear"]) -> None: ... | ||
| foo(' | ||
| # ^ | ||
| "#; | ||
| let report = | ||
| get_batched_lsp_operations_report_allow_error(&[("main", code)], get_default_test_report()); | ||
|
|
||
| // We expect the completion to NOT insert extra quotes if we are already in a quote. | ||
| // Currently it likely inserts quotes. | ||
| println!("{}", report); | ||
| assert!(report.contains("inserting `apple`"), "Should insert unquoted apple"); | ||
| assert!(report.contains("inserting `pear`"), "Should insert unquoted pear"); | ||
| } | ||
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.
is it worth including this line? let's keep the comment but remove the no-op
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.
sure i will remove the line 68 but leave the comment there?
is there a problem with the code tho?
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.
no issue with the code. but replacing something with itself (a no-op) isn't worth doing here imo
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.
yeah that makes sense ofc. i will look into this