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

Analyze completion site re: trailing parens #680

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Jan 29, 2025

Addresses posit-dev/positron#1818
Addresses posit-dev/positron#2338
Closes #369 (I just came across this earlier effort that stalled out in draft from)

I think of this as a "big and dumb" solution to this problem. But it works! The discussion I want to have is basically hinted at in #1818. I suspect it's time to create some formal CompletionOption struct that gets formed (probably) in provide_completions() that contains some analysis of the completion site. This will then be passed down to all the lower level functions that marshal completions.

Summary on what we decided together:

  • We will proceed with a big small and dumb solution along these lines, to enjoy immediate improvements in our completions for ? and inside debug() and friends.
  • Open new issue for the more profound changes that are needed in the long-term to enable completions that are responsive to the completion site and how it's situated in the AST. The "don't add parentheses and parameter hints" feature from this PR is one (but not the only) capability that the new approach will facilitate. Done in Refactor some infrastructure for completions #681.
  • Add tests to this PR.
  • Remove all/most of logging before merging. (The refactoring alluded to above will allow us to build in logging in a more streamlined way. It will be nice to make it possible to dump all completions with their provenance.)

Comment on lines +17 to +18
},
"sourceLanguages": ["rust"]
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a neutral-to-positive move for us.

use crate::treesitter::NodeTypeExt;
use crate::treesitter::UnaryOperatorType;

#[allow(dead_code)]
Copy link
Member Author

@jennybc jennybc Jan 30, 2025

Choose a reason for hiding this comment

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

Several of these fields are not actually used, but they were used along the way, i.e. when I had more logging in. I imagine this entire struct will go away when we do #681, so seems ok to have around for now.

}
}

pub fn check_for_function_value(context: &DocumentContext, node_context: &NodeContext) -> bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

I revised these to be less nested -- I liked that aspect of #369 once I discovered it.

.clone()
}

#[test]
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to test this here after all, as opposed to testing individual completion sources. It feels like we just want to know that the completion is correct, regardless of which helper ultimately provided it. It didn't seem to make sense to test the helpers themselves, since there's nothing particularly tricky about them correctly using the no_trailing_parens boolean. Again #681 is going to present a big opportunity to revisit testing that relates to "is the CompletionItem correct and is it provided by the expected source(s)?"

Copy link
Member Author

Choose a reason for hiding this comment

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

Used the basic structure of a test I found in #369.

@jennybc jennybc marked this pull request as ready for review January 30, 2025 23:52
@jennybc jennybc requested a review from DavisVaughan January 30, 2025 23:52
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.

1 participant