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

nixd: path completion #530

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

Conversation

Origami404
Copy link
Collaborator

Support completion for path expression.

image

@Origami404 Origami404 requested a review from inclyc as a code owner June 24, 2024 15:24
@inclyc inclyc changed the title [nixd] Path completion nixd: Path completion Jun 24, 2024
@inclyc inclyc changed the title nixd: Path completion nixd: path completion Jun 24, 2024
libnixf/test/Parse/Lexer.cpp Show resolved Hide resolved
nixd/lib/Controller/Completion.cpp Outdated Show resolved Hide resolved
}
}

// otherwise, fallback to VLACompletionProvider
Copy link
Member

Choose a reason for hiding this comment

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

You can determine whether or not providing VLA completion based on the upExpr result. For example if an ExprPath is encountered, then provide path completion, otherwise if ExprVar, provide VLA completion. Try to determine expr kind first, may make the source code more concise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When writing code, I believe that in future there will be more and more different completion modes added, and ultimately form something like that:

image

IMO the nested levels of codes will be too deep if we just adding if, so I refactor this part of code. But since we only have 3 completion modes at this time, I will revert changes here and use nested if.

Another solution here will be some gotos.

Copy link
Member

Choose a reason for hiding this comment

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

IMO the nested levels of codes will be too deep if we just adding if, so I refactor this part of code. But since we only have 3 completion modes at this time,

These three completion modes are switchable by the "context" of nix codes. For example, I suppose the attrpath completion only makes sense if there is actually an attrpath user typing, and path completion only makes sense if we are in attrpath.

Another solution here will be some gotos.

No, we are not writing automata.

Different platform will sort directory entry differently, change CHECK-NEXT to CHECK.
// if we are in a literal path, use PathCompletionProvider
if (Parent->kind() == Node::NK_ExprPath) {
const auto &Path = static_cast<const nixf::ExprPath &>(*Parent);
if (Path.parts().isLiteral()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (Path.parts().isLiteral()) {
if (Path.parts().isLiteral())

remove this curly brace?

fs::make_absolute(CurFileDir, Path);
}

if (fs::exists(Path) && fs::is_directory(Path)) {
Copy link
Member

Choose a reason for hiding this comment

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

can we do this in std::filesystem?

@inclyc inclyc added the enhancement New feature or request label Jun 25, 2024
@Aleksanaa
Copy link
Collaborator

Also <foo/bar> is a path (in $NIX_PATH). Of course we can also implement it later

@louisdutton
Copy link

louisdutton commented Oct 28, 2024

Is this really the responsibility of the nix lsp? This generic functionality is available through a variety of plugins such as https://marketplace.visualstudio.com/items?itemName=ionutvmi.path-autocomplete

If you're going ahead with this, how do you plan to reconcile with other plugins providing the same completions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants