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

Add navigation/find usages/completion support for Nix paths #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kef
Copy link

@kef kef commented May 31, 2022

  • Navigation (Ctrl-B/Cmd-B) for Nix paths and path-like strings.
    • Directories containing a default.nix file offer that file as an optional destination.
  • Find usages for files and directories in project view referenced by Nix paths.
  • Completion of files and subdirectories for Nix paths.

Note that rename for path components is partially working but gets an error. This needs more work, but doesn't impact on the other new features.

@kef kef changed the title Added navigation/completion/rename support for Nix paths. Add navigation/find usages/completion support for Nix paths Jun 1, 2022
Copy link
Contributor

@JojOatXGME JojOatXGME left a comment

Choose a reason for hiding this comment

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

First, thanks for your contribution. Here is my initial review.
I would appreciate if you could also write some basic test, if possible.
Maybe this might help https://plugins.jetbrains.com/docs/intellij/writing-tests-for-plugins.html.

Comment on lines +27 to +28
psiElement(NixStringText.class)
.withText(string().matches(NIX_PATH_REGEX))
Copy link
Contributor

Choose a reason for hiding this comment

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

I currently don't quite understand how this is working. How does this code associate which files match against which strings or paths? The sting might also contain escape sequences, shouldn't we have to parse them somewhere?

Comment on lines 191 to +194
string_text ::= STR | IND_STR
{
mixin="org.nixos.idea.psi.impl.NixStringReferencingElementImpl"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide some rationale why you are putting the mixin at string_text, not string? One string might contain multiple string_text elements, couldn't this be a problem? I fear that the current implementation would work for ./ in ''hi ''$./'', but not in ''*hi ./''. (I think we should probably detect neither of these cases, and only match against the whole string.)

Copy link

Choose a reason for hiding this comment

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

Hi I am in good hands I just need some money I am going back in tomorrow and I'm trying my luck and I'm not even trying

@JojOatXGME
Copy link
Contributor

Note that rename for path components is partially working but gets an error. This needs more work, but doesn't impact on the other new features.

What kind of error do we get? Is it possible that we catch the error and provide some more meaningful error message for the user?

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.

3 participants