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

cargo add: Infer crate name from path/git repo #83

Merged
merged 19 commits into from
Apr 9, 2016
Merged

cargo add: Infer crate name from path/git repo #83

merged 19 commits into from
Apr 9, 2016

Conversation

mseri
Copy link
Contributor

@mseri mseri commented Mar 18, 2016

This is an attempt to address issue #14.
There are still few FIXME in the code (in part due to me waiting for the hyper PR to be merged).

Add a dependency to a Cargo.toml manifest file.
Add a dependency to a Cargo.toml manifest file. If <crate> is a github or gitlab repository url, or
a local path, `cargo add` will try to authomaticall get the crate name and authomatically set the
appropriate `--git` or `--path`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Tiny typo: It's spelled 'automatically'. Also, please write URL in uppercase.)

@killercup
Copy link
Owner

Thank you!

I just had a quick look at this—It looks good so far (I added a comment about a typo), but I haven't tested it locally. This changes cargo-add quick significantly, so I would like to have some more tests for this.

There are still few FIXME in the code (in part due to me waiting for the hyper PR to be merged).

I'm not sure this PR (#67) will be merged. but I would like to refactor the fetch code in the future. No need to wait for this, though.

Regarding your Option FIXMEs: I would very much like to use Results for all error handling!Option should only indicate missing values, never errors. Feel free to refactor any code that doesn't follow this rule. 😄 (Also, quick-error is a pretty easy way to create comprehensive error enums.)

I'll be on vacation the next weeks, I'll probably check in every few days, though.

@mseri
Copy link
Contributor Author

mseri commented Mar 18, 2016

Thanks for the comments!

I will probably not be able to do much until Tuesday, I will push the changes concerning Option/Result as soon as possible. And then I will implement some new tests, do you have some that you would particularly like to see?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e5f3ad8 on mseri:add-recognise-path-v1 into * on killercup:master*.

@mseri
Copy link
Contributor Author

mseri commented Mar 25, 2016

I have cleaned the code, replaced Option to Result, and modified how I deal with paths -- although I am not convinced that I am doing it in the best way (I have left a FIXME there).

I also added tests concerning success and failure. If you think that we should add more, just let me know what you believe it's missing. Setting the environment variable "NO_REMOTE_CARGO_TEST" allows to skip the new tests requiring a working network connection.

This version seems to work on my computer (osx 10.11) using multirust with stable and beta. I don't understand where the SIGSEV in travis-unstable comes from. Do you have any guess?

@mseri
Copy link
Contributor Author

mseri commented Apr 4, 2016

@killercup any comment on this PR? I am using it since the latest commit and had no issue yet

@killercup
Copy link
Owner

Sorry for the radio silence, I didn't have as much time as I had hoped…

I'm not sure setting a NO_REMOTE_CARGO_TEST environment variable is the best way to do this. Rust's build-in testing framework allows you to mark tests as #[ignore], and later only run those with cargo test -- --ignored. Using this, we can easily separate tests using external APIs. Another option would be to add a test-external-apis cargo feature. Both are rather self-explainatory—and opt-in.

I don't understand where the SIGSEV in travis-unstable comes from. Do you have any guess?

No idea. Don't worry about it, I'll probably bump the nightly version later anyway to get a newer clippy.

@@ -0,0 +1,7 @@
[package]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename this to Cargo.toml.sample? We renamed all the other Cargo.toml files in the fixtures directory because it was confusing cargo install. Thanks!

@mseri
Copy link
Contributor Author

mseri commented Apr 9, 2016

No problem.

I am going to add a cargo feature then.
The SIGSEV was due to libcurl. It has been enough to bump the version to get rid of it.

Edit: I have updated the PR incorporating your comments.

@killercup
Copy link
Owner

Cool, thank you! Let's land this PR now 😄

@homu r+

@homu
Copy link
Contributor

homu commented Apr 9, 2016

📌 Commit d227f01 has been approved by killercup

@homu
Copy link
Contributor

homu commented Apr 9, 2016

⚡ Test exempted - status

@homu homu merged commit d227f01 into killercup:master Apr 9, 2016
homu added a commit that referenced this pull request Apr 9, 2016
`cargo add`: Infer crate name from path/git repo

This is an attempt to address issue #14.
There are still few FIXME in the code (in part due to me waiting for the hyper PR to be merged).
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.

4 participants