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

Complete tagged template application #7278

Merged
merged 15 commits into from
Feb 6, 2025

Conversation

nojaf
Copy link
Collaborator

@nojaf nojaf commented Feb 5, 2025

I noticed that

sh`echo "meh"`.

in https://github.com/zth/rescript-bun wasn't completing.
In this PR I'm adding the support for dot completion to be picked up from a frontend/syntax point of view.

This doesn't fully solve my problem but is useful nonetheless.

Comment on lines 1155 to 1156
(* This is likely to be an exprhole *)
| _ -> ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could add the explicit match (or an if branch using the utils we have for expr holes) just to make the code a bit clearer in what we care about in the AST. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not against having an explicit match, but I don't know what else to return when it is anything else, that is why I went with | _ -> "".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that'd remain, you'd just also add the explicit match for clarity, even though it's redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I split them after al.
Seems most clear to me.

Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

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

This all looks great! Just 2 smaller comments.

Also, this needs backporting to the VSCode extension repo as well after merge.

@nojaf nojaf requested a review from zth February 6, 2025 08:26
Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

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

Awesome, great work!

@zth zth merged commit d153a91 into rescript-lang:master Feb 6, 2025
21 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