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

Should we try to minimize the amount of keywords? #179

Open
blindFS opened this issue Dec 30, 2024 · 6 comments
Open

Should we try to minimize the amount of keywords? #179

blindFS opened this issue Dec 30, 2024 · 6 comments

Comments

@blindFS
Copy link
Contributor

blindFS commented Dec 30, 2024

Background:

there are many special rules of internal commands in tree-sitter-nu ATM, which have not much difference to general commands, such as

  • overlay
  • register
  • do
  • loop
  • error make

This can cause errors when users accidentally shadow them with customized definitions, e.g.

https://github.com/nushell/nu_scripts/blob/a83a40dff05e91daf90bb42e7a23c5e70c85a759/sourced/misc/nu_defs.nu#L492

It is actually a common issue among tree-sitter parsers, official doc suggests using keyword-extraction to handle it.
But it's not feasible in tree-sitter-nu, at least for now, since we already have identifier as word and we can't ignore the subtle differences between cmd_identifier and identifier.

So, what I'm suggesting here is removing those trivial rules and reduce them all into the command category.
As for the highlighting, we can still make them highlighted as keywords if we want to.

@fdncred
Copy link
Collaborator

fdncred commented Dec 30, 2024

My overall goal would be for someone to be able to use tree-sitter-nu as a nushell parser and recreate nushell if they wanted to. I'd like it to be at least that accurate.

So, I'm fine with reducing into the command category as long as it meets the first overall goal.

My second overall goal is to have excellent syntax highlighting so, i wouldn't want any change to affect that.

@blindFS
Copy link
Contributor Author

blindFS commented Dec 30, 2024

In terms of semantic accuracy/completeness, I think it's better to keep as it is.
Maybe because I never dig deep into the nushell's parser, the boundary between internal commands and the language essentials is a little bit obscure to me.

scope commands | where category == core generates even more stuff. Just wondering how to tell the difference.

@fdncred
Copy link
Collaborator

fdncred commented Dec 30, 2024

I wouldn't worry about categories too much, they can change pretty easily.

  • custom are custom commands like def blah [] {}
  • keyword are usually created in the nushell parser aka parser keywords
  • built-in are every other commands built into the nu binary
  • plugin are separate binaries starting with nu_plugin_ that are currently registered
  • external are custom completions
  • alias are aliases
 scope commands | group-by type
╭──────────┬──────────────────╮
 custom    [table 137 rows] 
 keyword   [table 34 rows]  
 built-in  [table 396 rows] 
 plugin    [table 163 rows] 
 external  [table 110 rows] 
╰──────────┴──────────────────╯

help commands breaks this out a little differently, which is probably a bug.

 help commands | group-by command_type
╭──────────┬──────────────────╮
 custom    [table 137 rows] 
 keyword   [table 34 rows]  
 built-in  [table 396 rows] 
 alias     [table 170 rows] 
 plugin    [table 163 rows] 
 external  [table 110 rows] 
╰──────────┴──────────────────╯

@blindFS
Copy link
Contributor Author

blindFS commented Dec 30, 2024

I overlooked the type column I guess, it's hidden behind ... XD.

Some of the rules that are not in the list of scope commands | where type == keyword

  • register
  • do
  • error make
  • hide-env
  • let-env

Surprised to see source-env export-env let hide are keywords, but let-env and hide-env are not.

I guess we are safe to remove the first 3 rules from the grammar?

@fdncred
Copy link
Collaborator

fdncred commented Dec 30, 2024

let-env and regsiter aren't commands anymore. do, error make, hide-env are still there in category core as type built-in. Having said that, I'm not surprised of some inconsistencies.

@blindFS
Copy link
Contributor Author

blindFS commented Dec 30, 2024

I think we can on hold the cleansing task since it's a breaking change that won't bring much benefit.

But this thread can be used to keep track of the obsolete rules.

And we can clean them from time to time when the list gets long.

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

No branches or pull requests

2 participants