add --offline mode fallback to cargo fmt #3813
Conversation
|
Not entirely sure how to test, as I can't readily simulate networking issues in my env (disabling the internet connection doesn't make a difference) but the |
calebcartwright
changed the title
src/cargo-fmt/main.rs
Outdated
| @@ -112,6 +116,18 @@ fn execute() -> i32 { | |||
| } | |||
| } | |||
|
|
|||
| let mut cargo_metadata_additional_opts: Vec<String> = Vec::new(); | |||
There was a problem hiding this comment.
I elected to go this route to make it easier to add any additional options in the future
There was a problem hiding this comment.
As for tests, there is tests/cargo-fmt but tests there got ignored in PR #3690.
However, a test could be added to it, even if it needs to be ignored at the moment I think until the problem above is resolved. At least a test would exists ;o)
Maybe cargo tests could be an inspiration ?
src/cargo-fmt/main.rs
Outdated
| @@ -112,6 +116,18 @@ fn execute() -> i32 { | |||
| } | |||
| } | |||
|
|
|||
| let mut cargo_metadata_additional_opts: Vec<String> = Vec::new(); | |||
I was hoping to be able to reproduce the networking issues referenced in #3811 to validate that including Will add some tests to this though, at least to validate the new |
src/cargo-fmt/main.rs
Outdated
| "--message-format", | ||
| "short", | ||
| "--", | ||
| "--edition", | ||
| "2018", | ||
| ]); | ||
| assert_eq!(true, o.quiet); | ||
| assert_eq!(true, o.offline); |
There was a problem hiding this comment.
I think that unit test is good enough... @topecongiro ?
There was a problem hiding this comment.
It'd be cool if one day we could have tests that validate that the correct options were passed to the cargo_metadata lib based on command line items passed to cargo fmt, though that seems like it might be a bit tricky
There was a problem hiding this comment.
Thank you for the PR! I was not aware of cargo --offline, it seems to be a good addition to cargo-fmt.
Instead of adding --offline command-line option, I prefer to use the offline mode of cargo metadata by default, and if we somehow fail, then fallback to the online mode. I think there is no real benefit of having an option to switch between offline or online mode, it feels like something that should be hidden to users.
SGTM, will work on making that switch. I'm not very familiar with cargo's Would it be better to first try |
|
@calebcartwright You may have already read it, but I found the blog post by nrc which explains the cargo
Good point, I agree that trying the online mode first is a safer approach. We could switch the default to the offline mode once we are aware of the side effect of doing so. |
Done! |
calebcartwright
changed the title
|
Thanks! |
Closes #3811