-
-
Notifications
You must be signed in to change notification settings - Fork 735
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
LSP: Add extra new line after 1st import autocomplete #3325
Conversation
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.
Thank you. Could you update the changelog file and add some tests please?
c7ef9f2
to
51aba4e
Compare
The existing tests cover these cases - both with newlines and without newlines. |
51aba4e
to
bf291ec
Compare
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.
This will not be merged until all the PR feedback has been resolved. Dedicated tests that demonstrate specifically the behaviour is not optional, with comments if they do not express the intent clearly enough by themselves.
7bf69e7
to
cd4efb7
Compare
..._core__language_server__tests__completion__importable_adds_extra_new_line_if_no_imports.snap
Show resolved
Hide resolved
Done! Added tests. |
.find_map(get_import) | ||
.map_or(0, |i| i.location.start); | ||
fn first_import_line_in_module(&'a self) -> (Position, bool) { | ||
let first_import = self.module.ast.definitions.iter().find_map(get_import); |
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.
Sorry- we always want imports to be added at the top, so could you make this check the first definition rather than searching for the first import? Thank you
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.
Hey, no worries. I used the first defnition. Please take a look.
It's weird that the first definition is always "import" statement. So it didn't change the behavior 🤷 .
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.
Ok, apparently we can just use the location.start
to find the 1st definition :)
cd4efb7
to
e1bc6c6
Compare
c67821e
to
cb06e34
Compare
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.
Beautiful, thank you. I'll update the changelog
I love your profile picture @Zhomart ! 😁 |
Not sure what happened to this PR: #3277
But here's a fix for #3271
Changes:
Demo (update Jul 13):