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

feat: update LabelCompleter with # for RecordSelect #7916

Merged
merged 24 commits into from
Jun 24, 2024

Conversation

jaschdoc
Copy link
Member

@jaschdoc jaschdoc commented Jun 20, 2024

Related to #7897

@jaschdoc jaschdoc force-pushed the label-completer-record-hash branch from 8ec4d72 to 720188c Compare June 20, 2024 12:32
@jaschdoc
Copy link
Member Author

I'm not quite sure why this does not work yet. Is there some other place that I have to update things for LSP to work?

@magnus-madsen
Copy link
Member

This may be enough. You can rebase, build a JAR and try it out.

@jaschdoc
Copy link
Member Author

I did that locally with the updated parser but it didn't seem to work :/

@magnus-madsen
Copy link
Member

I did that locally with the updated parser but it didn't seem to work :/

Can you paste the program you used? Also double check if # has to be escape in regex world.

@magnus-madsen
Copy link
Member

Also, in the previous regex, what did [] do?

@jaschdoc
Copy link
Member Author

jaschdoc commented Jun 21, 2024

I did that locally with the updated parser but it didn't seem to work :/

Can you paste the program you used? Also double check if # has to be escape in regex world.

The # symbol is not used for anything according to the java documentation https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html so I think it's fine to use as a literal. I think [] was used to use . as a literal since it can mean any character if it's not escaped or used in [].
The original regex was (.*)[.].* updated to (.*)#.* meaning any character zero or more times, then a hash, then any character zero or more times.
I also tried (.*)[#].* but IntelliJ suggested I remove the [] since it means any of the 1 characters.

@jaschdoc
Copy link
Member Author

Here is my test program:

def main(): Unit \ IO =
    let r = { abcde = { bcdef = { cdefg = 2 } }, x = 3 };
    let s = r.abcde.bcdef.cdefg;
    println(s)

@jaschdoc
Copy link
Member Author

jaschdoc commented Jun 21, 2024

Screenshot from 2024-06-21 11-11-49

Also the current impl is wrong since it suggests labels that do not exist. It should only suggest r.abcde.bcdef.

@magnus-madsen
Copy link
Member

Also the current impl is wrong since it suggests labels that do not exist. It should only suggest r.abcde.bcdef.

That is a known limitation, so its an orthogonal issue.

Perhaps # does not trigger autocomplete? I seem to remember there is something called autocomplete chars somewhere. Maybe in the VSCode repo? But I guess you can trigger it manually and still not see anything?

@jaschdoc
Copy link
Member Author

Yeah, hitting ctrl+space to load suggestions just gives all functions in the std lib

@jaschdoc
Copy link
Member Author

jaschdoc commented Jun 21, 2024

Okay here's a weird bug: I'm using the updated compiler that I built locally where the updated parser is merged but it still runs the above program just fine when using . instead of #... how? It even says in the output that it's using the jar that I put in the root of the vscode project.

@magnus-madsen
Copy link
Member

You need to make a change in Weeder2 too... See the original ticket.

@jaschdoc
Copy link
Member Author

jaschdoc commented Jun 21, 2024

Ah ok I misunderstood what you meant so I reverted the weeder change in the last PR

@jaschdoc
Copy link
Member Author

Note (to self): There may be something with triggerCharacters. Try searching for it on https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion

@jaschdoc
Copy link
Member Author

I will do some manual testing once flix/vscode-flix#419 is merged

@magnus-madsen
Copy link
Member

I will do some manual testing once flix/vscode-flix#419 is merged

I pushed a new version of the extension.

@jaschdoc
Copy link
Member Author

It works now!

Screenshot from 2024-06-23 15-34-56

@jaschdoc jaschdoc marked this pull request as ready for review June 23, 2024 13:36
@magnus-madsen magnus-madsen merged commit 6c60adc into flix:master Jun 24, 2024
12 checks passed
@jaschdoc jaschdoc deleted the label-completer-record-hash branch June 24, 2024 07:04
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.

None yet

2 participants