PathSegmentsMut has no finish method like UrlQuery

[seanmonstar]

When building a URL, it would be convenient to be able to finish mutating path segments like one currently can for query pairs.

// will NOT compile
let mut url = Url::parse("...");
url
    .path_segments_mut().unwrap()
        // ...
    .query_pairs_mut()
        // ...
        .finish()
    // ...

Instead the query pairs must be mutated first, since this 'sub builder' can be finished, returning a Url. Which (though slightly counter-intuitive) is fine, it's just a slightly jarring restriction.

It also means the mutable variable is necessary; if it behaved like query_pairs_mut then this could all be on one chain which could be passed directly (or via an immutable variable) into a Client method.

From seanmonstar/reqwest#150
/cc @OJFord

[StaloneLab]

Would like to work on this, but don't really know how to implement properly, should I implement Target for PathSegmentsMut or create a specific finish() method for PathSegmentsMut, which will output an Url?

[SimonSapin]

@TitiAlone No, the Target trait is a hack that I wish weren’t part of the public API. It allows using application/x-www-form-urlencoded serialization without a Url, which does not apply to PathSegmentsMut. You can add an inherent finish method that returns &'a mut Url (not Url).

[StaloneLab]

Just worked on this today and here's the code I made (included the doc because it's important).

/// Returns the URL reference the PathSegmentsMut was constructed with.
///
/// Example:
///
/// ```rust
/// use url::Url;
/// # use std::error::Error;
///
/// # fn run() -> Result<(), Box<Error>> {
/// let mut url = Url::parse("https://github.com/servo/rust-url/issues/363")?;
/// url.path_segments_mut()
///    .map_err(|_| "cannot be base")?
///    .clear()
///    .finish()
///    .query_pairs_mut()
///    // ...
///    .finish();
/// assert_eq!(url.as_str(), "https://github.com/");
/// # Ok(())
/// # }
/// # run().unwrap();
/// ```
pub fn finish(&mut self) -> &mut ::Url {
    self.url
}

This makes an error, not on cargo build but on cargo test, returning this:

---- src\path_segments.rs - path_segments::PathSegmentsMut<'a>::finish (line 222) stdout ----
        error: borrowed value does not live long enough
  --> <anon>:14:13
   |
8  | / url.path_segments_mut()
9  | |    .map_err(|_| "cannot be base")?
   | |__________________________________- temporary value created here
...
14 |      .finish();
   |               ^ temporary value dropped here while still borrowed
   |
   = note: values in a scope are dropped in the opposite order they are created
   = note: consider using a `let` binding to increase its lifetime

[tmccombs]

finish needs to take ownership of (and consume) self. Otherwise the PathSegmentsMut would continue to hold onto a mutable reference to the Url while simaltaneously returning a mutable reference to it.

pub fn finish(self) -> &'a mut ::Url {
    self.url
}

[StaloneLab]

tmccombs: Try by yourself, it will not work, because we would need .clear() (for instance, but any other method should) to return a mut PathSegmentMut, and not a &mut PathSegmentMut in order to chain methods.

[tmccombs]

You're right. I don't think it is unreasonable to change clear & comp. to take self instead of &mut self, except that it is backwards incompatible (although I have no idea what the actual impact of such a change would be).