-
Notifications
You must be signed in to change notification settings - Fork 327
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
Move completions to new testing infrastructure #64
Conversation
I just checked out your branch and got it working with CURSOR, that seems to fix the StringIndexOutOfBounds error. Will soon push a fix! |
The presentation compiler expects "_CURSOR_" to be inserted in the code where the current cursor is located. This seems to help completions at positions like `List.<COMPLETE>`, without _CURSOR_ the presentation compiler throws a StringIndexOutOfBoundsException.
1689820
to
06d1e50
Compare
I'm still not happy with the code organization for the compiler. It seems we should delegate to CompletionProvider/SignatureHelpProvider/HoverProvider to create the compilation unit, since they have different needs for CURSOR. I will try to refactor things a bit more. |
Each service has different needs for how the CompilationUnit and Position are constructed. Previously we replicated a lot of critical logic in the test suites, which made it difficult to keep the server in sync with tests.
I thought this was a regression from the compiler refactoring, but it turns out it had always been a bug.
I hope this makes sense. There is still quite a bit of duplication for the three providers that can be abstracted away by changing the superclass LanguageServer, but we can leave that for later. The signature help bug fix is there because I thought it was a regression from the refactorings, sorry for mixing that together. |
PS, can you maybe open PRs from your personal fork @gabro ? it seems CI runs test on every push to any branch on origin and I get email notifications when those tests fail. |
Seems CI is unhappy :(
Will try to restart tomorrow! |
- languageserver.Compiler is now compiler.ScalacProvider - remove unused parameters to ScalacProvider - remove unused classes
de0a9a5
to
5ce1ee7
Compare
@olafurpg ah sure, I've been opening on the main repo out of habit at work, but I didn't realize Travis would notify you. I'll use my fork from now on. Also, thanks for the nice refactoring. I think it makes sense to delegate responsibility over the compiler configuration to the single providers. |
Having a well-defined API between the server and Global will become even more important once we add support for multiple scala versions via classloading. Travis is happy now :D |
I'm also used to open PRs from the branches in the repo itself at work (isn't it the main point for giving the collaborator permissions?). But I don't mind either way. Anyway, if it's just about Travis notifications, I usually just rurn them off: notifications:
email: false Or it can be configured for only certain emails and only certain events: notifications:
email:
recipients:
- one@example.com
- other@example.com
on_success: never # default: change
on_failure: change # default: always Also docs say that
So the problem is in deduplicating the triggering events: |
I think there's no need to complicate the Travis settings, if the alternative is using a fork model for PRs. I wouldn't lose too much time on this :) |
Another aspect is that I typically try to keep the number of branches in the origin fork clean. Contributors who fork the repo pull all branches and I don't like it I fetch 100s of WIP branches (many forgotten and will never get merged) when contributing to a project. |
Makes sense 👍 |
Still WIP.
This closes #59. I've picked up @olafurpg's work and simply changed the implementation to use
safeCompletionsAt
. I'll add some extra test cases and merge this, so that we can use it as a base for #62 and #63