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

Fixed anonymous struct completion #23

Merged
merged 5 commits into from
Oct 21, 2016
Merged

Conversation

RSDuck
Copy link
Contributor

@RSDuck RSDuck commented Oct 21, 2016

I've put some research into the data the language server spits out and it seemed like the extension never gets informations about the field names.

The reason for this is, that the extension asks the language server for top level completion. The fix was pretty easy, I just had to edit the function which checks wether to use top level completion.

This should fix https://github.com/vshaxe/haxe-languageserver/issues/21

@Gama11
Copy link
Member

Gama11 commented Oct 21, 2016

From that diff it looks like you accidentally reverted some changes I made in da80752.

@Gama11
Copy link
Member

Gama11 commented Oct 21, 2016

Also, this seems to stop working with the test case from HaxeFoundation/haxe#3907 if there is a space before the cursor after the opening {.

Works:
doStuff({|

Doesn't work:
doStuff({ |

static function calculateCompletionPosition(text:String, index:Int):CompletionPosition {
text = text.substring(0, index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry. I'm not very used to the code base

@@ -36,7 +36,7 @@ class CompletionFeature {
}

static var reFieldPart = ~/(\.|@(:?))(\w*)$/;
static var reStructPart = ~/[(,]\s*{\s*((\s*\w+\s*:\s*["'\w()\.]+\s*,\s*)*\w*)$/;
static var reStructPart = ~/[(,]\s*{(\s*(\s*\w+\s*:\s*["'\w()\.]+\s*,\s*)*\w*)$/;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to move the \s into the capture group for the whitespace fix to actually work.

@Gama11
Copy link
Member

Gama11 commented Oct 21, 2016

Thanks, seems to work nicely now!

I don't particularly like that completion needs to be triggered manually here, maybe we can improve that in the future.

@Gama11 Gama11 merged commit 6fa2284 into vshaxe:master Oct 21, 2016
@RSDuck RSDuck deleted the anonymous_struct_fix branch October 21, 2016 19:32
@RSDuck
Copy link
Contributor Author

RSDuck commented Oct 21, 2016

I just noticed that the pattern has some serious flaws(I should have tested it on real code). For example when you have a structure containing other structures(it doesn't work recursively). Also when having operators in function it also doesn't work.

Here some examples:

test({
twoSquared: 2*2, // Breaks suggestion
person: new Person({firstName: "Bill", surname: "Gates"}) // Breaks also the suggestions for the following code
});

@Gama11
Copy link
Member

Gama11 commented Oct 22, 2016

Yeah, deciding whether to trigger field or toplevel completion actually gets quite complex in the case of struct completion, I don't think a regex will do long-term... For metadata and field completion it's not so bad.

Any ideas how Haxe could assist with this @Simn / @nadako?

@Simn
Copy link
Member

Simn commented Oct 22, 2016

Even the compiler doesn't know that you're declaring a structure until it sees the :, so this is always going to be an issue.

@nadako
Copy link
Member

nadako commented Apr 19, 2017

Today I found out that the reStructPart regexp added in 6fa2284 is so heavy it can hang the language server completely for minutes in a large file. I don't quite understand how does it work, so I'm inclined to revert that change for now... I'll try taking a closer look at it, but in general, regex hacks like these should go away when we use hxparser and incremental compilation.

nadako added a commit that referenced this pull request Apr 19, 2017
Gama11 added a commit that referenced this pull request May 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants