Skip to content
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

fix: wrong "open" completion position with namespace #1300

Merged
merged 2 commits into from
May 24, 2024

Conversation

MrLuje
Copy link
Contributor

@MrLuje MrLuje commented May 23, 2024

Fix code completion for external namespace being inserted before namespace and bring back Completion.AutoOpen tests

before :
fix-open-namespace-before

after :
fix-open-namespace-after

NB: I changed the position for Namespace.fsx & NamespaceWithNewLine.fsx to match the actual behavior, it is correct F# syntax but I would have expected it to be aligned with the code below instead of the namespace (at least we get coverage for the inserted line part)
fix-open-namespace-nested

NB2: Since I added back Completion.AutoOpen tests, I can't get the full test suite to run locally, the test adapter is crashing with OOM. Maybe the dedicated createServer function missed some "recent" optimisations applied to others createServer since it was not running until now ?

Currently, "open" are sometimes inserted before namespace which is invalid + fix "Completion.AutoOpen" tests
@baronfel
Copy link
Contributor

Wow yeah, the stack on macOS is much smaller so you can clearly see the crashes there.

Since the script files are all logically independent, the server could probably be shared across the tests entirely if you want to try that out.

@MrLuje
Copy link
Contributor Author

MrLuje commented May 24, 2024

Yep, much better !
Thanks

@baronfel
Copy link
Contributor

Thanks for this great fix @MrLuje!

@baronfel baronfel merged commit 610d0ab into ionide:main May 24, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants