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

update keyword and operator definitions in highlights.scm #156

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

Conversation

fdncred
Copy link
Collaborator

@fdncred fdncred commented Nov 28, 2024

This PR is an attempt to update the keywords, operators and commands. I'm hoping that things like str replace are highlighted as a command instead of being highlighted as a command and an argument.

I'm not sure how to test this. Any guidance would be helpful.

Maybe commands with spaces aren't allowed?

@blindFS
Copy link
Contributor

blindFS commented Nov 28, 2024

I think it's not possible without changing the grammar. And even if we manage to make builtin internals correct, there're still user defined composite commands... I'm afraid there's no perfect solution for this in tree-sitter.

@fdncred
Copy link
Collaborator Author

fdncred commented Nov 28, 2024

I got the CI to pass at least. I guess I need to point my nvim config at this branch to get it to work/use it?

@blindFS
Copy link
Contributor

blindFS commented Nov 28, 2024

I got the CI to pass at least. I guess I need to point my nvim config at this branch to get it to work/use it?

Actually you can just edit your highlights.scm file in the ftplugin to test the color, but I think nothing will change, since the parsed tree is the same.

@fdncred
Copy link
Collaborator Author

fdncred commented Nov 28, 2024

@blindFS
Copy link
Contributor

blindFS commented Nov 28, 2024

I mean the local path that your plugin manager installs tree-sitter-nu into, in my case, it's ~/.local/share/nvim/lazy/tree-sitter-nu/queries/nu, I've tested the command head, no diff:

image

@fdncred
Copy link
Collaborator Author

fdncred commented Nov 28, 2024

ya, right. it's pointing to my local where the file is edited.

darn. so, the grammar has to be changed to make the highlighting work right? i guess that means that the grammar doesn't recognize commands with spaces.

I guess i don't understand this part of the highlights.scm if it does nothing. why is it even in the file?

@blindFS
Copy link
Contributor

blindFS commented Nov 28, 2024

@fdncred it does the job of correct highlighting the "head" if it is in the collected strings, however the replace is parsed as argument already, it's out of head, so won't be affected.

image

@blindFS
Copy link
Contributor

blindFS commented Nov 28, 2024

Sorry, you're right, (cmd_identifier) already get it highlighted, the second rule is totally redundant.

@fdncred
Copy link
Collaborator Author

fdncred commented Nov 28, 2024

would be nice if the grammar for nushell would be able to highlight commands in nushell. LOL

Seems like the parser would need to check if the text after a command was part of the command or not and if not it's an argument, otherwise it's all the command. Maybe that's easier to say than to do?

@blindFS
Copy link
Contributor

blindFS commented Nov 28, 2024

@fdncred The only way that I can imagine is hardcoded choices in command_rule:

function _command_rule(parenthesized) {
  return (/** @type {any} */ $) => {
    const sep = parenthesized ? $._separator : $._space;
    return prec.right(
      seq(
        choice(
          field("head", seq(optional(PUNC().caret), $.cmd_identifier)),
          field("head", seq(PUNC().caret, $._stringish)),
          field("head", choice(...INTERNALS())), // <-- collection of all composite commands
        ),
        repeat(seq(sep, optional($._cmd_arg))),
      ),
    );
  };
}

But still, it can't deal with user-defined ones (probably no solution).

@fdncred
Copy link
Collaborator Author

fdncred commented Nov 28, 2024

I'm not as concerned about custom commands, but internal commands and plugins should at least be identified as such.

@mrdgo
Copy link
Contributor

mrdgo commented Nov 28, 2024

I see two options:

  1. full enumeration of all the commands with spaces within the grammar
  2. Retrieve the commands with spaces from upstream
    Both have pros and cons, I'd open a discussion with the treesitter people, they might know, if 2. is even possible.

Edit: I have a slight aversion against manually maintaining all these commands in this repo

@fdncred
Copy link
Collaborator Author

fdncred commented Nov 28, 2024

Edit: I have a slight aversion against manually maintaining all these commands in this repo

I understand that but I'd also "keep it simple". Having the commands and plugin command hard coded somehow seems easy to maintain because that section could just be replaced as needed. This is what the vscode extension does here in the match line. A tool was written to generate this regex monstrosity.

@mrdgo
Copy link
Contributor

mrdgo commented Nov 28, 2024

Edit: I have a slight aversion against manually maintaining all these commands in this repo

I understand that but I'd also "keep it simple". Having the commands and plugin command hard coded somehow seems easy to maintain because that section could just be replaced as needed. This is what the vscode extension does here in the match line. A tool was written to generate this regex monstrosity.

We could also use the regex, then we only sync regexes and don't have to maintain different representations of the same information. What do you think?

@fdncred
Copy link
Collaborator Author

fdncred commented Nov 28, 2024

I'm up for trying it. If we don't like it, we can always revert it.

@blindFS
Copy link
Contributor

blindFS commented Nov 29, 2024

@fdncred naive implementation FYI.

Update: extra regex suffix for situations like weatherdark.nu where http get_api has overlap with internal http get, causing a parsing error each time http get_api is called without the fix.

@fdncred
Copy link
Collaborator Author

fdncred commented Nov 29, 2024

With vscode, we can make regexes with ^/$ and \b word boundaries. Would be nice to be able to combine INTERNAL with word boundaries so it knows where words end. Also, can you put up a PR with your implementation. I think I could test it easier by checking out your PR.

@blindFS
Copy link
Contributor

blindFS commented Nov 29, 2024

With vscode, we can make regexes with ^/$ and \b word boundaries. Would be nice to be able to combine INTERNAL with word boundaries so it knows where words end. Also, can you put up a PR with your implementation. I think I could test it easier by checking out your PR.

Sad story, \b is not supported in tree-sitter grammar

Error processing rule command_token1

Caused by:
    Regex error: Assertions are not supported

fdncred added a commit that referenced this pull request Nov 29, 2024
For test purpose #156

---------

Co-authored-by: Darren Schroeder <[email protected]>
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