Skip to content

Stop excluding base::return() and base::function from completions #768

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

Merged
merged 2 commits into from
Apr 9, 2025

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Apr 8, 2025

I'm opening Completion Month with a real banger! Let's stop explicitly excluding base::return() and base::function from completions.

Relates to posit-dev/positron#4842

I've

  • Thought about this 🤔
  • Looked through relevant comments and git blame in ark
  • Looked at what RStudio does

and I can't find any reason to not just let base::return() and base::function show up in our completions. They will both be contributed by our search path completions, coming from base.

Both are represented by snippets, which I have some mixed feelings about. So I plan to discuss some additional improvements relating to the snippet treatment of function and return(). We could also discuss posit-dev/positron#1850 at the same time. But perhaps this simple fix can happen first, especially for return().

@jennybc jennybc requested review from DavisVaughan and lionel- April 8, 2025 22:08
@@ -53,7 +53,7 @@ fn completions_from_search_path(
let mut completions = vec![];

const R_CONTROL_FLOW_KEYWORDS: &[&str] = &[
"if", "else", "for", "in", "while", "repeat", "break", "next", "return", "function",
"if", "else", "for", "in", "while", "repeat", "break", "next",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to exclude any of these actually? The shorter ones won't be triggered if they are below the character limit for completions but it presumably wouldn't hurt to include them.

Copy link
Member Author

Choose a reason for hiding this comment

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

As we discussed in person, I suspect it's not good that we exclude these at all. I will open a new issue shortly with a systematic look at how reserved words currently fare in positron's R completions. And that will lead to some new proposals.

@kevinushey
Copy link
Contributor

Does Ark provide the appropriate escaping so that the completion from base:: gives base::`function`?

@jennybc
Copy link
Member Author

jennybc commented Apr 9, 2025

@kevinushey No. That is already on my radar and will appear soon in its own issue or added to an existing issue. I definitely bumped into this while playing around for this PR.

@jennybc jennybc merged commit 58f1fc7 into main Apr 9, 2025
13 checks passed
@jennybc jennybc deleted the bugfix/rescue-completion-for-return-and-function branch April 9, 2025 17:41
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants