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

Quote module/function/record names in completion, signature help #71

Closed
wants to merge 3 commits into from

Conversation

the-mikedavis
Copy link
Contributor

This improves the display of signature help and fixes label (and edit) values for completions of functions or record names that need quoting, for example #'basic.publish'{} from the Rabbit codebase or calling a function on an Elixir module from Erlang.

Also included is a refactor of base_db::to_quoted_string to return Cow<str> instead, avoiding allocation in the common case that an identifier doesn't need quoting. This saves a few unnecessary allocations.

@robertoaloi
Copy link
Contributor

Thanks! Would it make sense to add a testcase for the completion or the signature help showcasing the change?

@the-mikedavis
Copy link
Contributor Author

Good call! I added snapshot tests for completion and signature help. While looking at tests I noticed that completion seems to not work for remote modules that need quoting, for example functions on a Elixir.<ModuleName> module. I can look into that too but I think it will be separate from these changes

@facebook-github-bot
Copy link
Contributor

@robertoaloi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@robertoaloi
Copy link
Contributor

Amazing, thanks for this!
Not related to this PR, but we should consider change our general approach around names. Right now, we have to explicitly perform the quoting every time, something that (as you noticed) is easy to forget. Ideally, we'd have a central solution for it.

We discussed briefly with @michalmuskala and we're currently using the Name type for both variables and atoms, two entities with different requirements. A possibility would be to split the type definition, which would then make it possible to enforce correctness and avoid passing raw strings around.

@facebook-github-bot
Copy link
Contributor

@robertoaloi merged this pull request in 7af8893.

@the-mikedavis the-mikedavis deleted the md/quote-names branch December 18, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants