Merged
Conversation
Contributor
|
I'm going to play with this a bit. Nice POC! |
Contributor
|
Wow! Awesome idea; thank you for putting this together. Fewer panics in this area will be a big UX improvement. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8123 +/- ##
===========================================
+ Coverage 54.77% 67.83% +13.05%
===========================================
Files 613 620 +7
Lines 98494 99758 +1264
===========================================
+ Hits 53954 67670 +13716
+ Misses 44540 32088 -12452
|
d898eac to
6612aad
Compare
d709674 to
d28b470
Compare
Contributor
|
@kubouch Related to:
I'm definitely up for landing this and seeing how these edge cases manifest themselves. It's kind of hard to tell by reading these if there are deal-breakers or not. |
Contributor
|
Let's go ahead and land, and we'll use the time between now and the next release to polish it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds an alternative alias implementation. Old aliases still work but you need to use
old-aliasinstead ofalias.Instead of replacing spans in the original code and re-parsing, which proved to be extremely error-prone and a constant source of panics, the new implementation creates a new command that references the old command. Consider the new alias defined as
alias ll = ls -l. The parser creates a new command calledlland remembers that it is actually alscommand called with the-lflag. Then, when the parser sees thellcommand, it will translate it tols -land passes to it any parameters that were passed to the call toll. It works quite similar to how known externals defined withexternare implemented.The new alias implementation should work the same way as the old aliases, including exporting from modules, referencing both known and unknown externals. It seems to preserve custom completions and pipeline metadata. It is quite robust in most cases but there are some rough edges (see later).
Fixes #7648 #8026 #7512 #5780 #7754
No effect: #8122 (we might revisit the completions code after this PR)
Should use custom command instead: #6048
User-Facing Changes
Since aliases are now basically commands, it has some new implications:
alias spam = "spam"(requires command call)alias spam = echo "spam"def foo [] { 'foo' }; alias foo = ls -l(foo defined more than once)alias ls = (ls | sort-by type name -i)whichcommand,$nu.scope.aliases, etc.alias f = f)alias ls = ls -a)aliasdoesn't list aliases anymorealias ou = overlay usewon't work)Tests + Formatting
Don't forget to add tests that cover your changes.
Make sure you've run and fixed any issues with these commands:
cargo fmt --all -- --checkto check standard code formatting (cargo fmt --allapplies these changes)cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collectto check that you're using the standard code stylecargo test --workspaceto check that all tests passAfter Submitting
If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.