Command alias or Alias command #1091 #2679

sbeckeriv · 2016-05-12T18:37:36Z

Dearest Reviewer,

This pull request closes #1091 which is a request to support aliases.
This is not as powerful as something like git's alias, however, I think
it sticks true to the original request.

I high jack the processing of the args. After a few flags are checked
and the args are parsed I check the config file for alias.COMMAND. If it
is there I split it like args does and replace args[1](the original
command) with the alias command and its 'flags'.

As an extra measure I output the alias command with warn. I would be
willing to drop that or put it behind a verbose flag. Also the docs have
been updated.

Thanks!
Becker

rust-highfive · 2016-05-12T18:37:47Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

bors · 2016-05-12T20:51:15Z

☔ The latest upstream changes (presumably #2396) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2016-05-13T20:14:32Z

Thanks for the PR! The implementation looks pretty clean to me, I'd just probably elide the warning that you're using an alias and instead let it silently go through.

Curious what others think about this though (cc @rust-lang/tools)

sbeckeriv · 2016-05-14T00:45:33Z

Removed warning. thanks!

sbeckeriv · 2016-05-19T14:36:00Z

Any more thoughts on this pull request?

Thanks
Becker

michaelwoerister · 2016-05-19T14:39:11Z

We discussed this in the tools subteam meeting and, as far as I remember, the consensus was that we are generally in favor of this idea.
@alexcrichton should have more details.

alexcrichton · 2016-05-19T23:03:35Z

Ok, got a chance to talk about this some more with @wycats today as well (in addition to @rust-lang/tools). We were all somewhat ambivalent (neither here nor there on the feature), but some points brought up were:

We probably want to canonicalize on a few well-known aliases immediately. That is, perhaps cargo b/r/t could stand in for build/run/test for everyone.
This runs a risk of having project disagree on common aliases, but it somewhat mitigated by the point above.
This probably also wants to allow aliases of the form foo = ["bar", "baz with spaces"] to handle arguments-with-spaces
We may want to probe the filesystem as well when activating an alias to ensure that the corresponding cargo subcommand doesn't exist. If it does, we probably want to run the subcommand instead.

sbeckeriv · 2016-05-20T06:40:19Z

We may want to probe the filesystem as well when activating an alias to ensure that the corresponding cargo sub-command doesn't exist. If it does, we probably want to run the sub-command instead.

Lets say my project has a .cargo/config
[alias] fast-test = ["test", "--fast"]
On my computer I have no extra cargo commands and fast-test runs well. On your computer you have installed the new fast-test cargo command which does things differently. Our commands will not match up. Another example might be different format flags for different sub-crates. I think for consistency the config alias should win. You also can not set default flags for known commands. I am not sure how the config code pulls the different layers together. Does my root alias section get merged with the project's alias section?

Maybe the --list command can show that there is a sub-command and alias for debugging?

sbeckeriv · 2016-05-20T16:20:40Z

I moved to a list for the alias and noticed a fun issue. I have a test file at the root of the project and in the test I create a test config. I found that the command list was being merged when I had overlapping alias. I think a non-merging get_list might be a good idea for this. Thoughts?

alexcrichton · 2016-05-23T15:20:44Z

src/bin/cargo.rs

+            default_alias.insert("t", "test".to_string());
+            default_alias.insert("r", "run".to_string());
+            let mut args: Vec<String> = env::args().collect();
+            let command = args[1].clone();


Does this need to be cloned here? The if let below I believe shouldn't borrow it for too long

The clone is needed because I assign args[1] in the if block. Turns out I do not really need the command variable. I am now passing args[1] directly to the get.

alexcrichton · 2016-05-23T15:22:10Z

Perhaps, yeah, although we'd probably want to specify the strategy when fetching the key of how to deal with duplicate arrays. The paths override list, for example, definitely wants merging, whereas you're right in that this probably does not.

sbeckeriv · 2016-05-25T05:44:13Z

I was about done with the code when you said to ignore the quotes. Please checkout the regex code. It does not handle unbalanced quotes. I was thinking a warning "using an alias with unbalanced quotes. this may produce unexpected results please review your alias or ignore this message". since it is most likely an error case it should not come up often.

I did spend some time trying to find the command line arg parser to reused but I could not find it.

thanks again!

bors · 2016-05-26T06:05:56Z

☔ The latest upstream changes (presumably #2743) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2016-05-31T17:18:41Z

src/bin/cargo.rs

+            match value {
+                Some(record) => {
+                    // Group quoted items together
+                    let regex = Regex::new("(\"[^\"]*\"|[^\"]+)").unwrap();


Oh sorry, what I meant here was something like:

If the configuration is a string, then we just split on whitespace

We also accept a configuration of a list of strings, in which case we do not splitting. In that case I think we can avoid using a regex here

I can support both string and list. I like the string regex because its more user friendly and what someone might expect. There is no easy feedback for the final command that is ran. If someone needed to use the list version it might be hard to debug why the string is failing. Should I keep the regex and support the list or figure out an easy way to display the command that is ran?

brson · 2016-05-31T18:29:25Z

Feels very feature-creepy to me. Aliases are also something that can be solved in the shell.

sbeckeriv · 2016-05-31T18:33:53Z

Ok. Thanks for the time.
Becker

brson · 2016-05-31T18:38:07Z

Let's not close just yet. Still worth considering closely.

sbeckeriv · 2016-06-01T19:39:17Z

I was just looking at my gitconfig aliases. I was thinking about the usefulness of the current limitation and if the shell should handle the aliases. I think only one of my git aliases uses direct shell commands. Its not clear how I would personally use cargo alias other than to shorten names. I an see shell aliases that do more than just run cargo commands. If I would need to run more then one command would I bother with the cargo alias commands? I can see that running a custom build by default that lives outside the used shell scripts would be nice for consistency.

I was thinking that the whole thing was "feature-creepy". Are you talking about something specific in the pull request is feature creep or the whole thing?

alexcrichton · 2016-06-05T07:41:49Z

Note that I don't consider this equivalent to shell aliases because they crucially can't do what this PR is doing. A shell alias typically takes a non-whitespace-separated string (e.g. foo-bar) and translates it to some other possibly whitespace-separated string (e.g. foo bar). Aliases in git (or for Cargo here) allow you to type git foo which can't be done through a shell alias. Additionally, this is a consistent way to set a Cargo alias if that's your goal, rather than hunting down shell configs.

I also use git aliases all the time (e.g. git co instead of git checkout), and I personally like them. I don't really see this as feature creep because it seems well motivated by other tools and isn't exactly something we're starting out by inventing.

sbeckeriv · 2016-06-20T17:41:47Z

Sorry I got distracted. @alexcrichton you would like me to move back to the list version of the config and drop the string splitting or support both?

Thanks again
Becker

alexcrichton · 2016-06-24T16:34:59Z

@sbeckeriv at minimum we should support a string list and then I think it's also optional that we could support string configuration which is only split on spaces.

sbeckeriv · 2016-07-01T04:33:50Z

Updated. Both now string and list works. I removed the regex that tried to not split quoted parts of a string. If someone needs more complex splitting they can fall back to the list version.

Thanks again. I have learned a lot!
Becker

alexcrichton · 2016-07-05T18:18:57Z

src/bin/cargo.rs

@@ -186,6 +207,31 @@ fn execute(flags: Flags, config: &Config) -> CliResult<Option<()>> {
    Ok(None)
 }

+fn list_of_alias_commands(config: &Config, command: &String) -> Option<Vec<String>> {


I think this'll want to return a CargoResult of the output so malformed configuration can be printed as well.

i see. If the alias is an float for example. The code current ignores that.

Thanks! I will sort this out. I think I want to use a CargoResult<Option<Vec<_>>>.

alexcrichton · 2016-07-05T18:20:16Z

Thanks @sbeckeriv! The implementation looks good to me. @brson do you have specific concerns you're worried about that you could elaborate on?

brson · 2016-07-05T18:48:39Z

@alexcrichton No.

Dearest Reviewer, This pull request closes rust-lang#1091 which is a request to support aliases. This is not as powerful as something like git's alias, however, I think it sticks true to the original request. I high jack the processing of the args. After a few flags are checked and the args are parsed I check the config file for alias.COMMAND. If it is there I split it like args does and replace args[1] (the original command) with the alias command and its 'flags'. I have also included default short hand commands b, t and r. Thanks! Becker

sbeckeriv · 2016-07-07T04:39:03Z

@alexcrichton Thanks. I added a test for the mismatched type error. I updated the docs to include the defaults, a list example and a white space example. I was unsure of a good place to document the default aliases. Should I update the usage message? Something like:
build Compile the current project (aliased as b)

Also I don't know what changed but building cargo on nightly is blazing fast! The broken up test are also way faster!

alexcrichton · 2016-07-13T18:27:00Z

@bors: r+

Ok cool, thanks @sbeckeriv! I think we may want to explore documenting this in the --help messages eventually, but for now let's see how this plays out in terms of rolling it out first. Thanks again for keeping up with the PR!

bors · 2016-07-13T18:27:02Z

📌 Commit 66739f1 has been approved by alexcrichton

bors · 2016-07-13T18:36:45Z

⌛ Testing commit 66739f1 with merge 61885fb...

Command alias or Alias command #1091 Dearest Reviewer, This pull request closes #1091 which is a request to support aliases. This is not as powerful as something like git's alias, however, I think it sticks true to the original request. I high jack the processing of the args. After a few flags are checked and the args are parsed I check the config file for alias.COMMAND. If it is there I split it like args does and replace args[1] (the original command) with the alias command and its 'flags'. As an extra measure I output the alias command with warn. I would be willing to drop that or put it behind a verbose flag. Also the docs have been updated. Thanks! Becker <img width="784" alt="screen shot 2016-05-12 at 10 23 59 am" src="https://cloud.githubusercontent.com/assets/12170/15226012/d18a3336-1835-11e6-94c9-875a63a79856.png">

bors · 2016-07-13T19:36:27Z

☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64
Approved by: alexcrichton
Pushing 61885fb to master...

rust-highfive assigned alexcrichton May 12, 2016

sbeckeriv force-pushed the alias branch 3 times, most recently from 989602d to a9d1aef Compare May 13, 2016 15:21

sbeckeriv force-pushed the alias branch from a9d1aef to c8a3462 Compare May 14, 2016 00:44

sbeckeriv force-pushed the alias branch from c8a3462 to c11eb3b Compare May 20, 2016 16:16

sbeckeriv force-pushed the alias branch 2 times, most recently from dc477c5 to c8ae752 Compare May 21, 2016 00:57

alexcrichton reviewed May 23, 2016
View reviewed changes

sbeckeriv force-pushed the alias branch 2 times, most recently from 68819c3 to ed12743 Compare May 25, 2016 05:37

sbeckeriv force-pushed the alias branch from ed12743 to bcfe6f3 Compare May 31, 2016 17:13

alexcrichton reviewed May 31, 2016
View reviewed changes

sbeckeriv closed this May 31, 2016

brson reopened this May 31, 2016

sbeckeriv force-pushed the alias branch 2 times, most recently from 17c15f8 to 2d0a587 Compare July 1, 2016 04:18

sbeckeriv force-pushed the alias branch from deb8e47 to a65f943 Compare July 4, 2016 18:17

alexcrichton reviewed Jul 5, 2016
View reviewed changes

sbeckeriv force-pushed the alias branch 2 times, most recently from b4c167b to 681ef6c Compare July 7, 2016 04:24

sbeckeriv force-pushed the alias branch from 681ef6c to 66739f1 Compare July 7, 2016 04:31

alexcrichton added the relnotes Release-note worthy label Jul 13, 2016

bors merged commit 66739f1 into rust-lang:master Jul 13, 2016

sbeckeriv deleted the alias branch July 13, 2016 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command alias or Alias command #1091 #2679

Command alias or Alias command #1091 #2679

sbeckeriv commented May 12, 2016

rust-highfive commented May 12, 2016

bors commented May 12, 2016

alexcrichton commented May 13, 2016

sbeckeriv commented May 14, 2016

sbeckeriv commented May 19, 2016

michaelwoerister commented May 19, 2016

alexcrichton commented May 19, 2016

sbeckeriv commented May 20, 2016

sbeckeriv commented May 20, 2016

alexcrichton May 23, 2016

sbeckeriv May 23, 2016

alexcrichton commented May 23, 2016

sbeckeriv commented May 25, 2016

bors commented May 26, 2016

alexcrichton May 31, 2016

sbeckeriv May 31, 2016

brson commented May 31, 2016

sbeckeriv commented May 31, 2016

brson commented May 31, 2016

sbeckeriv commented Jun 1, 2016

alexcrichton commented Jun 5, 2016

sbeckeriv commented Jun 20, 2016

alexcrichton commented Jun 24, 2016

sbeckeriv commented Jul 1, 2016

alexcrichton Jul 5, 2016

sbeckeriv Jul 7, 2016

alexcrichton commented Jul 5, 2016 •

edited

Loading

brson commented Jul 5, 2016

sbeckeriv commented Jul 7, 2016

alexcrichton commented Jul 13, 2016

bors commented Jul 13, 2016

bors commented Jul 13, 2016

bors commented Jul 13, 2016

Command alias or Alias command #1091 #2679

Command alias or Alias command #1091 #2679

Conversation

sbeckeriv commented May 12, 2016

rust-highfive commented May 12, 2016

bors commented May 12, 2016

alexcrichton commented May 13, 2016

sbeckeriv commented May 14, 2016

sbeckeriv commented May 19, 2016

michaelwoerister commented May 19, 2016

alexcrichton commented May 19, 2016

sbeckeriv commented May 20, 2016

sbeckeriv commented May 20, 2016

alexcrichton May 23, 2016

Choose a reason for hiding this comment

sbeckeriv May 23, 2016

Choose a reason for hiding this comment

alexcrichton commented May 23, 2016

sbeckeriv commented May 25, 2016

bors commented May 26, 2016

alexcrichton May 31, 2016

Choose a reason for hiding this comment

sbeckeriv May 31, 2016

Choose a reason for hiding this comment

brson commented May 31, 2016

sbeckeriv commented May 31, 2016

brson commented May 31, 2016

sbeckeriv commented Jun 1, 2016

alexcrichton commented Jun 5, 2016

sbeckeriv commented Jun 20, 2016

alexcrichton commented Jun 24, 2016

sbeckeriv commented Jul 1, 2016

alexcrichton Jul 5, 2016

Choose a reason for hiding this comment

sbeckeriv Jul 7, 2016

Choose a reason for hiding this comment

alexcrichton commented Jul 5, 2016 • edited Loading

brson commented Jul 5, 2016

sbeckeriv commented Jul 7, 2016

alexcrichton commented Jul 13, 2016

bors commented Jul 13, 2016

bors commented Jul 13, 2016

bors commented Jul 13, 2016

alexcrichton commented Jul 5, 2016 •

edited

Loading