Modernize all the things #129
Conversation
ordian
force-pushed
the
modernize
branch
4 times, most recently
from
6c053c5
to
66a0bcd
Compare
There was a problem hiding this comment.
Thanks!
So, I was pretty annoyed when I saw one single commit with +490 −279 stats… but luckily 267 of these lines are Cargo.lock and whole bunch are just try->?, so it's okay. It'll still take me a while to read through ti, though 😅
But before I do that: What's you motivation for switching to reqwest? I'm totally fine with having less C-based dependencies but I'll need more reason than that. :)
Does this:
- Work on Windows? (cf. Does cargo-edit work on Windows? #55)
- Allow us to specify a request timeout?
- Can we get nice error messages? (cf. 'cargo add' crashes without a direct internet connection #110)
| @@ -72,8 +73,8 @@ dependencies (version set to "*"). | |||
| "#; | |||
|
|
|||
| fn handle_add(args: &Args) -> Result<(), Box<Error>> { | |||
| let mut manifest = try!(Manifest::open(&args.flag_manifest_path.as_ref().map(|s| &s[..]))); | |||
| let deps = try!(args.parse_dependencies()); | |||
| let mut manifest = Manifest::open(&args.flag_manifest_path.as_ref().map(|s| &s[..]))?; | |||
There was a problem hiding this comment.
Memo to self: Hm, I think I know a better way to get this working by now, .as_ref().map(|s| &s[..]) looks quite ugly.
src/bin/add/fetch.rs
Outdated
| @@ -247,6 +232,29 @@ quick_error! { | |||
| } | |||
| } | |||
|
|
|||
| macro_rules! get_crate_name_from_repository { | |||
There was a problem hiding this comment.
Nice touch with the macro, but… can we make that a function with a signature like (repo: &str, matcher: &Regex, url_template: Fn(user: &str, repo: &str) -> String)?
src/bin/add/fetch.rs
Outdated
| } | ||
|
|
||
| quick_error! { | ||
| #[derive(Debug)] | ||
| pub enum FetchVersionError { | ||
| Curl(err: curl::Error) { | ||
| Reqwest(err: reqwest::Error) { | ||
| from() | ||
| description("Curl error") | ||
| display("Curl error: {}", err) |
|
Also cc @bjgill who just updated to curl 0.4 |
ordian
force-pushed
the
modernize
branch
6 times, most recently
from
abc8bf5
to
e6c224b
Compare
Reqwest should work on windows (although I can't test it now) and is more concise than curl.
Yup.
Not sure about that, but I hope so. |
|
I was largely updating curl in the hopes that it'd then respect the It looks as if reqwest has recently added proxy support (seanmonstar/reqwest#152), but not (yet) released it. |
| })?; | ||
|
|
||
| fn get_default_timeout() -> Duration { | ||
| Duration::from_secs(10) |
There was a problem hiding this comment.
Do you think 10 seconds for the timeout is a sensible default?
There was a problem hiding this comment.
10 seconds sounds good. What's the error message we get when the timeout was hit?
(Also, can you add new changes as separate commits in the future? We can always squash them later.)
There was a problem hiding this comment.
We will get FetchVersionError::Reqwest "Https error: timed out"
(Sorry about squashing)
|
I've no objection to the move - I think we'd be wanting to do it eventually anyway.
|
|
Alright, let's get this merged. I'll also open an issue to track/add tests for nice error output. bors r+ |
Build succeeded |
Sorry, my commits were squashed during stashing.