Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Generalize this into cargo-edit #3

Closed
wants to merge 14 commits into from

Conversation

killercup
Copy link
Contributor

Hi, I spent some time this morning fiddling with cargo-add to enable it to edit other fields (dev- and build-deps).

This pretty much works now, although the CLI commands are quite long (cargo edit build-deps add gcc). I've also done some cleanups and smaller things, like adding travis config and an .editorconfig file.

Since this is a pretty big change, I don't expect you to merge this PR as-is (or at all). I just wanted to let you know :)

@killercup killercup force-pushed the feature/generalize branch from 18d0d97 to 72d552e Compare June 20, 2015 15:56
@withoutboats
Copy link
Owner

Sorry it took me a while to respond, my mail server is down so I didn't get notified about this PR.

First, wow! that you were even willing to work with the terrible code I wrote for the first pass of this project (meaning the overbearing monad-passing style of the actual project & the hacky shell test). I was not writing for accessibility or extensibility, so congrats.

Second, I was heading in much the same direction myself, though it looks like from a different angle (I was writing a strict manifest parser that requires that the Cargo.toml met the 'spec' at doc.crates.io/manifest.html), so hopefully I'll be able to integrate what you've written with what I've written. I'll pull your code into my local repo some time in the next few days and check it out in more detail.

If you have thought out a hierarchy of commands, feel free to write it up either just in this convo or in markdown file in the main directory. One I know I want is a fix subcommand that would (possibly interactively) assist in cleaning up an invalid manifest.

@killercup
Copy link
Contributor Author

Your code wasn't that bad… also, everything I added is probably the same
kind of hack (don't look at the error handling if you can help it).

Also, I've added some more stuff in my master branch (like that tree
command I mentioned on reddit), so you might want to have a look at that as
well. Sadly, I can't change this PR's source.

Lee Aronson [email protected] schrieb am Mo., 22. Juni 2015 um
07:33:

Sorry it took me a while to respond, my mail server is down so I didn't
get notified about this PR.

First, wow! that you were even willing to work with the terrible code I
wrote for the first pass of this project (meaning the overbearing
monad-passing style of the actual project & the hacky shell test). I was
not writing for accessibility or extensibility, so congrats.

Second, I was heading in much the same direction myself, though it looks
like from a different angle (I was writing a strict manifest parser that
requires that the Cargo.toml met the 'spec' at [
doc.crates.io/manifest.html] http://doc.crates.io/manifest.html%5D), so
hopefully I'll be able to integrate what you've written with what I've
written. I'll pull your code into my local repo some time in the next few
days and check it out in more detail.


Reply to this email directly or view it on GitHub
#3 (comment).

@Turbo87
Copy link

Turbo87 commented Aug 18, 2015

@killercup wouldn't it be better to split this into multiple commands? maybe something like this:

$ cargo add [--section=dependencies] dep1 [dep2...]
$ cargo list [--section=dependencies] [--tree]

@killercup
Copy link
Contributor Author

@Turbo87 I totally agree. Feel free to change this to produce multiple binaries :)

@withoutboats
Copy link
Owner

Hey! I've been meaning to pull this and work on it, and I'm going to try to make it a priority over the next week or 2.

Namespacing the commands I have no strong feelings about. Some people thought that adding a whole bunch of top level commands was not a great idea, and I'm inclined to agree - modulated by a preference for the brevity that doing that would provide. I'm leaning toward namespacing them all under cargo cfg, which has the advantage that the extra characters are adjacent and 2/3 of them are also in the word cargo, making it easier to rapidly touch type them in my opinion.

@withoutboats
Copy link
Owner

Oops!

@withoutboats withoutboats reopened this Aug 18, 2015
@killercup
Copy link
Contributor Author

Sounds awesome. Feel free to take any code I've written for this if you
like (or rewrite it to make it somewhat good). Do you want me to update
this PR so that it is up-to-date to my master branch?

I'm not sure about the cfg namespace. It's fast to type, but associated
it with configuring cargo itself. IMHO, cargo edit deps add regex still
sounds good (but is too long).

withoutboats [email protected] schrieb am Di., 18. Aug. 2015 um
23:55:

Hey! I've been meaning to pull this and work on it, and I'm going to try
to make it a priority over the next week or 2.

Namespacing the commands I have no strong feelings about. Some people
thought that adding a whole bunch of top level commands was not a great
idea, and I'm inclined to agree - modulated by a preference for the brevity
that doing that would provide. I'm leaning toward namespacing them all
under cargo cfg, which has the advantage that the extra characters are
adjacent and 2/3 of them are also in the word cargo, making it easier to
rapidly touch type them in my opinion.


Reply to this email directly or view it on GitHub
#3 (comment).

@killercup
Copy link
Contributor Author

Another fancy idea: abbreviations for power users. Just to see what it
would be like, I'm not saying we should implement that right now.

$ cargo e +regex
$ cargo e +logger --dev
$ cargo e -url
$ cargo ls deps
$ cargo ls --tree

withoutboats [email protected] schrieb am Di., 18. Aug. 2015 um
23:57:

Oops!


Reply to this email directly or view it on GitHub
#3 (comment).

@Turbo87
Copy link

Turbo87 commented Aug 19, 2015

@withoutboats I agree that "adding a whole bunch of top level commands was not a great idea", but so far I only suggested two and I think at least add is relevant enough justify it being a top level command. It is also what was suggested by @wycats in rust-lang/cargo#4.

@killercup killercup closed this Oct 11, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants