Allow arguments for the last flag in short flag batch by MariaSolOs · Pull Request #8808 · nushell/nushell

MariaSolOs · 2023-04-08T04:10:48Z

Description

Fixes #5923

Currently nushell doesn't allow short flag batches to contain arguments, despite this being a common pattern in commands like git commit -am 'My commit message'. This PR relaxes this so that the last flag in the batch can take an argument.

User-Facing Changes

nu::parser::short_flag_arg_cant_take_arg has been replaced by nu::parser::only_last_flag_in_batch_can_take_arg and is displayed when a flag other then the last in a short flag batch takes an argument.

Tests + Formatting

Both test_parser.rs and test_known_external.rs have been updated to test the new allowed and disallowed scenarios.

amtoine · 2023-04-08T17:34:46Z

👋

quite strangely, it does already work with git aliases 😮

i have

alias.cm=commit --verbose

and

git cm -am "foo"

works 😕

MariaSolOs · 2023-04-08T17:40:20Z

quite strangely, it does already work with git aliases 😮

@amtoine Very interesting, I have some of the aliases from nu_scripts and so I didn't consider something like alias.cm=commit --verbose.

In fact, could you further explain (or point me to the right docs) what alias.X does? I had not seen that before.

amtoine · 2023-04-08T18:06:39Z

quite strangely, it does already work with git aliases open_mouth

@amtoine Very interesting, I have some of the aliases from nu_scripts and so I didn't consider something like alias.cm=commit --verbose.

oooh i see what you're using 😋
true nushell aliases!

In fact, could you further explain (or point me to the right docs) what alias.X does? I had not seen that before.

i should have been clearer, sorry 😢
when i say i have alias.cm=commit --verbose, it means that i've configured git to run commit --verbose when i simply use the cm subcommand! (the alias.<alias>=... simply is the output of the git config --show command 😋)
these are built-in git aliases, really shortcuts if you will 😉

Note
you can see this one in my git config

MariaSolOs · 2023-04-08T19:10:31Z

i should have been clearer, sorry 😢
when i say i have alias.cm=commit --verbose, it means that i've configured git to run commit --verbose when i simply use the cm subcommand! (the alias.=... simply is the output of the git config --show command 😋)
these are built-in git aliases, really shortcuts if you will 😉

Oh my no worries at all! I was confused because I thought you were talking about nushell's alias hehe. Thanks for clarifying!

sholderbach · 2023-04-08T22:12:42Z

crates/nu-parser/src/parser.rs

-                orig.start + 1 + short_flag.0,
-                orig.start + 1 + short_flag.0 + 1,
-            );
+            let short_flag_span = Span::new(orig.start + 1 + i, orig.start + 1 + i + 1);


This line previously already made the assumption that a short flag is only an ASCII character and requires just one byte. Our parsing of definitions apparently doesn't check for that causing downstream panics. I opened an issue for that (#8821)

Ohh good catch!

@sholderbach would you like #8821 to get fixed first, or are you okay with merging this before?

nvm, it seems like #8849 fixed the issue!

Run cargo fmt Update tests to new short batch parsing

MariaSolOs · 2023-04-12T02:11:06Z

I've updated the PR to accommodate for the latest changes from #8849 :)

# Description Follow up to #8849 Work for #8821 Should unblock work on #8808 Missing is a restriction to printables or identifiers according to [UAX31](https://www.unicode.org/reports/tr31/) # User-Facing Changes Non ASCII characters should be allowed in shortflags. # Tests + Formatting Want to add some tests for the error reporting cases

- Include the span fixes from nushell#8866 - Include the `break;` back in - Remove unnecessary clone

sholderbach

The implementation and your tests look good to me. Thank you very much!

Should be easy to update to the state of #8866

crates/nu-parser/src/parser.rs

codecov · 2023-04-14T21:29:40Z

Codecov Report

Merging #8808 (de69f2a) into main (45d33e7) will increase coverage by 0.00%.
The diff coverage is 88.88%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8808   +/-   ##
=======================================
  Coverage   68.51%   68.51%           
=======================================
  Files         636      636           
  Lines      101657   101657           
=======================================
+ Hits        69647    69648    +1     
+ Misses      32010    32009    -1

Impacted Files	Coverage Δ
crates/nu-protocol/src/parse_error.rs	`7.79% <0.00%> (ø)`
crates/nu-parser/src/parser.rs	`85.35% <100.00%> (ø)`

... and 5 files with indirect coverage changes

MariaSolOs · 2023-04-15T02:23:54Z

@sholderbach For my own learning: Could you please explain what the 88.88% diff coverage refers to in the comment above? I'm reading the Codecov docs and all the other values in the summary table make sense to me, but I'm not understanding where that 88.88% is coming from...

MariaSolOs · 2023-04-15T02:31:08Z

crates/nu-parser/src/parser.rs

            let mut found_short_flags = vec![];
            let mut unmatched_short_flags = vec![];
            for (offset, short_flag) in short_flags.char_indices() {
-                let orig = arg_span;


There's no need to keep orig.

amtoine

looks good to me, that's a sweet improvement 😊

EDIT: just tested and it works like a charm ✨

sholderbach · 2023-04-15T08:24:11Z

@sholderbach For my own learning: Could you please explain what the 88.88% diff coverage refers to in the comment above? I'm reading the Codecov docs and all the other values in the summary table make sense to me, but I'm not understanding where that 88.88% is coming from...

I think it is the fraction of the added or changed lines that are measured to be covered. But the exact numbers can be weird depending on pure definitions or test modules

# Description Follow up to nushell#8849 Work for nushell#8821 Should unblock work on nushell#8808 Missing is a restriction to printables or identifiers according to [UAX31](https://www.unicode.org/reports/tr31/) # User-Facing Changes Non ASCII characters should be allowed in shortflags. # Tests + Formatting Want to add some tests for the error reporting cases

# Description _Fixes #5923_ Currently `nushell` doesn't allow short flag batches to contain arguments, despite this being a common pattern in commands like `git commit -am 'My commit message'`. This PR relaxes this so that the last flag in the batch can take an argument. # User-Facing Changes - `nu::parser::short_flag_arg_cant_take_arg` has been replaced by `nu::parser::only_last_flag_in_batch_can_take_arg` and is displayed when a flag other then the last in a short flag batch takes an argument. # Tests + Formatting - Both [`test_parser.rs`](https://github.com/nushell/nushell/blob/48af0ebc3c9f0c7b0ef134078fe1a0e47982059e/crates/nu-parser/tests/test_parser.rs#L640-L704) and [`test_known_external.rs`](https://github.com/nushell/nushell/blob/48af0ebc3c9f0c7b0ef134078fe1a0e47982059e/src/tests/test_known_external.rs#L42-L61) have been updated to test the new allowed and disallowed scenarios. --------- Co-authored-by: sholderbach <sholderbach@users.noreply.github.com>

MariaSolOs · 2023-04-15T17:14:22Z

I think it is the fraction of the added or changed lines that are measured to be covered. But the exact numbers can be weird depending on pure definitions or test modules

I still don't get it but all the diffs are green so I'm happy with that 😅

sholderbach · 2023-04-15T18:51:58Z

I still don't get it but all the diffs are green so I'm happy with that sweat_smile

Me neither 😆

sholderbach reviewed Apr 8, 2023

View reviewed changes

MariaSolOs force-pushed the short-flag-batch-with-arg branch from 2393d2e to 101404f Compare April 12, 2023 01:44

MariaSolOs marked this pull request as draft April 12, 2023 01:45

MariaSolOs force-pushed the short-flag-batch-with-arg branch from 101404f to 48af0eb Compare April 12, 2023 01:59

MariaSolOs added 2 commits April 11, 2023 19:05

Allow arg in last flag of short batch

ec56fdb

Update tests to new short batch parsing

3b55612

Run cargo fmt Update tests to new short batch parsing

MariaSolOs force-pushed the short-flag-batch-with-arg branch from 48af0eb to 3b55612 Compare April 12, 2023 02:07

MariaSolOs marked this pull request as ready for review April 12, 2023 02:08

sholderbach mentioned this pull request Apr 12, 2023

Fix span of multibyte short flags #8866

Merged

Merge branch 'main' into short-flag-batch-with-arg

0822c83

- Include the span fixes from nushell#8866 - Include the `break;` back in - Remove unnecessary clone

sholderbach reviewed Apr 14, 2023

View reviewed changes

crates/nu-parser/src/parser.rs Show resolved Hide resolved

crates/nu-parser/src/parser.rs Outdated Show resolved Hide resolved

Remove redundant variable

de69f2a

MariaSolOs commented Apr 15, 2023

View reviewed changes

amtoine approved these changes Apr 15, 2023

View reviewed changes

sholderbach merged commit 1d68c48 into nushell:main Apr 15, 2023

MariaSolOs deleted the short-flag-batch-with-arg branch April 15, 2023 15:12

Conversation

MariaSolOs commented Apr 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-Facing Changes

Tests + Formatting

Uh oh!

amtoine commented Apr 8, 2023

Uh oh!

MariaSolOs commented Apr 8, 2023

Uh oh!

amtoine commented Apr 8, 2023

Uh oh!

MariaSolOs commented Apr 8, 2023

Uh oh!

sholderbach Apr 8, 2023

Choose a reason for hiding this comment

Uh oh!

MariaSolOs Apr 9, 2023

Choose a reason for hiding this comment

Uh oh!

MariaSolOs Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

MariaSolOs Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

MariaSolOs commented Apr 12, 2023

Uh oh!

sholderbach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MariaSolOs commented Apr 15, 2023

Uh oh!

MariaSolOs Apr 15, 2023

Choose a reason for hiding this comment

Uh oh!

amtoine left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sholderbach commented Apr 15, 2023

Uh oh!

MariaSolOs commented Apr 15, 2023

Uh oh!

sholderbach commented Apr 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MariaSolOs commented Apr 8, 2023 •

edited

Loading

codecov bot commented Apr 14, 2023 •

edited

Loading

amtoine left a comment •

edited

Loading