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

Feat: internal commands with spaces #161

Merged
merged 3 commits into from
Nov 29, 2024
Merged

Feat: internal commands with spaces #161

merged 3 commits into from
Nov 29, 2024

Conversation

blindFS
Copy link
Contributor

@blindFS blindFS commented Nov 29, 2024

For test purpose #156

@fdncred
Copy link
Collaborator

fdncred commented Nov 29, 2024

Seems to work pretty good to me
image

@blindFS
Copy link
Contributor Author

blindFS commented Nov 29, 2024

@fdncred yes, it works, but the dirty hack somehow worries me a little bit :-)
Especially the suffix...

@mrdgo
Copy link
Contributor

mrdgo commented Nov 29, 2024

The main reason for me to like the regex idea is automation. But in this case, we can probably also write a *.nu script that creates the list of commands with spaces. Treesitter exists for javascript, so we can programatically extract the list and match it against the automatically created list of actual commands with spaces. Overall, I'd prefer this solution now, because it also results in easier to understand code.

@fdncred
Copy link
Collaborator

fdncred commented Nov 29, 2024

I just noticed that the INTERNAL function only has commands with spaces in them vs the entire list of commands. Is that because the single word commands are already captured elsewhere?

@blindFS
Copy link
Contributor Author

blindFS commented Nov 29, 2024

I just noticed that the INTERNAL function only has commands with spaces in them vs the entire list of commands. Is that because the single word commands are already captured elsewhere?

Yes, as the cmd_identifier.

@fdncred
Copy link
Collaborator

fdncred commented Nov 29, 2024

Also keep in mind that in order to get commands and plugins with spaces in them, you have to have the plugins built, installed, and registered for this command below to work. Also, that command would get multiword aliases and custom commands too.

scope commands | get name | where {' ' in $in} o> list

This is the command below is probably more appropriate. We could also include command_type == external and that adds whatever custom completions you have loaded. 🤔

# generated with Nu 0.100.1
help commands | 
   where command_type == built-in or command_type == plugin or command_type == keyword |
   get name | 
   where {' ' in $in}

@blindFS
Copy link
Contributor Author

blindFS commented Nov 29, 2024

Current implementation can't tell internals/externals from each other, if we want them highlighted differently, more complicated cmd_identifier is necessary.

@fdncred
Copy link
Collaborator

fdncred commented Nov 29, 2024

I'm not saying that they should be highlighted differently. I'm just saying that the help commands or scope commands only needs to list the appropriate things. e.g. custom commands should not be included in our list.

@blindFS
Copy link
Contributor Author

blindFS commented Nov 29, 2024

I'm not saying that they should be highlighted differently. I'm just saying that the help commands or scope commands only needs to list the appropriate things. e.g. custom commands should not be included in our list.

Sure, this pr is just a demo, we should redesign pipeline to generate the list.

@fdncred
Copy link
Collaborator

fdncred commented Nov 29, 2024

I have the rustup, cargo, git and gh custom completions loaded (along with keywords, builtins and plugins) and I have a list of 531 commands with spaces in them. I can provide that list or just update this PR before landing it.

@fdncred
Copy link
Collaborator

fdncred commented Nov 29, 2024

Sure, this pr is just a demo, we should redesign pipeline to generate the list.

I don't think we want to do that because it would require downloading and building plugins and downloading and installing custom completions. Building plugins takes forever in CI. The list should really change much and I'd personally most complete list than just bare nushell if possible.

@blindFS
Copy link
Contributor Author

blindFS commented Nov 29, 2024

Does your script above generate all 531 of those commands? If so, we can add that script along with the generated list file to the repo. But I guess only you can maintain that list right? We probably only get a small part of it with the same script?

@blindFS
Copy link
Contributor Author

blindFS commented Nov 29, 2024

We probably should test the impact on performance, 531 seems to be a significant number.

@fdncred
Copy link
Collaborator

fdncred commented Nov 29, 2024

agreed. i just pushed my list to the pr.

@blindFS
Copy link
Contributor Author

blindFS commented Nov 29, 2024

@fdncred Do we need cargo xxx in the list?
One test case violated by it, no big deal.

@fdncred
Copy link
Collaborator

fdncred commented Nov 29, 2024

We can remove any of them that we want. However, I'm not sure 19 commands will make any difference. These are just the custom completions I usually run with rustup, cargo, git and gh. Feel free to remove any of these. I mainly want the commands and plugin commands. I just thought it was a "nice to have" to have the rest.

@blindFS
Copy link
Contributor Author

blindFS commented Nov 29, 2024

@fdncred we can keep it and update the test case,

the performance loss is about 7~8%, 4757/5136 bytes/ms

@fdncred
Copy link
Collaborator

fdncred commented Nov 29, 2024

ok, let's run with this. it's just software. If we don't like how it works, we can always revert/remove it.

@fdncred fdncred merged commit 631a58f into nushell:main Nov 29, 2024
3 checks passed
@blindFS blindFS mentioned this pull request Nov 29, 2024
fdncred pushed a commit that referenced this pull request Nov 29, 2024
I was too slow on the thread of #161 , some necessary updates is not
pushed on-time.

1. commands start with `overlay` and `export` are removed, they are
treated as keywords
2. one test case with `cargo install` updated
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