LSP: Include braces in type import completions #3963
Conversation
There was a problem hiding this comment.
Looks really good! I've left one note about the API change in the tests. It'll need to not change the API the others use.
I think it would be best to write the code directly for this one case rather than have all the other tests and the macro adapt for this one unique case.
| @@ -601,15 +607,15 @@ impl<'a> TestProject<'a> { | |||
| pub fn at<T>( | |||
| &self, | |||
| position: Position, | |||
| new_src: Option<&str>, | |||
There was a problem hiding this comment.
Remove this please. The other tests should not change, we don't want to have a parameter that every test needs to provide but only this one uses.
| @@ -559,6 +559,7 @@ impl<'a> TestProject<'a> { | |||
| pub fn positioned_with_io( | |||
| &self, | |||
| position: Position, | |||
| new_src: Option<&str>, | |||
There was a problem hiding this comment.
Remove this please. The other tests should not change, we don't want to have a parameter that every test needs to provide but only this one uses.
I did that in my first attempt, but I thought we might have more tests with src being invalid syntax in the future? If you think no, I would just write the code for this test case
maybe we could offer two more functions |
They would do the same. It must not change the other tests
I think making helper functions for a single test case is premature, we can wait and see how many of these come up. |
Frank-III
force-pushed
the
type_completion
branch
from
a234d06 to
87e8b6c
Compare
close: #3498
continuation of #3516, to support tests with invalid syntax.
I am not sure if I should make it a different function. Please review it 🙏