`url::form_urlencoded::serialize` is moving iterated key-value pairs about rust-url HOT 26 CLOSED

This part of the API has not gotten a lot of use or thought yet. What specific input type do you think would work better here?

from rust-url.

I think if the signature would be changed to ...

// note the `&` in front of the tuple
// disclaimer: don't know if it actually compiles
pub fn serialize<'a, I>(pairs: I) -> String where I: Iterator<Item = &'a (&'a str, &'a str)> {

... then we should be good for my case.
While we are talking about it, it might be worth removing serialized_owned from the public API as well, just because it's unintuitive that it all of the sudden is that specific. Just making it generic might be worth doing alternatively.

from rust-url.

This signature works for you specific use case, but is less flexible: the outer & means you have to have existing tuples of &str somewhere, to reference them.

I’m starting to thing an iterator is not so great for this. What do you think of a method you call multiple times instead?

let mut s = String::new();
{
    let mut query = url::from_urlencoded::Serializer::new(&mut s);
    query.add_pair("client_id", "bogus");
    query.add_pair("scope", "A B C");
}

from rust-url.

The builder patterns seems like 'too much' in this case. It would require the serializer to keep its own, dynamically allocated state, which is less efficient than it should be. It's my believe that an API should not enforce unnecessary copies or allocations. Ideally, it's made so that it works out of the box for 80% of the use-cases, but can be adapted with a little effort (like .iter().map(|&(a,b)| (a, b)) in my case .. which is of course part of the 80% of the code ;)).

Here are my thoughts as code:

    fn pair_transformer<'a, I>(pairs: I) -> String
        where I: Iterator<Item=&'a (&'a str, &'a str)> {
        for pair in pairs {
            let &(a, b) = pair;
            // do something
        }
        String::new()
    }

    fn consuming_pair_transformer<'a, I>(pairs: I) -> String
        where I: Iterator<Item=(&'a str, &'a str)> {
        for pair in pairs {
            let (a, b) = pair;
            // do something
        }
        String::new()
    }

    // This works, natively
    pair_transformer([("a", "b")].iter());

    // This one too
    pair_transformer(vec![("a", "b")].iter());

    struct Pair<A: Copy, B: Copy> {
        first: A,
        second: B
    }

    impl<A: Copy, B: Copy> Pair<A, B> {
        fn from_tuple(t: (A, B)) -> Pair<A, B> {
            Pair {
                first: t.0,
                second: t.1
            }
        }

        fn as_tuple(&self) -> (A, B) {
            (self.first, self.second)
        }
    }

    // Consuming custom type instances
    let pairs = [Pair::from_tuple(("a", "b"))];
    // pair_transformer(pairs.iter().map(|p| &p.as_tuple()));
        //     tests/lang.rs:845:44: 845:56 error: borrowed value does not live long enough
        // tests/lang.rs:845     pair_transformer(pairs.iter().map(|p| &p.as_tuple()));
        //                                                              ^~~~~~~~~~~~
        // tests/lang.rs:845:22: 845:57 note: reference must be valid for the method call at 845:21...
        // tests/lang.rs:845     pair_transformer(pairs.iter().map(|p| &p.as_tuple()));
        //                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        // tests/lang.rs:845:43: 845:56 note: ...but borrowed value is only valid for the block at 845:42
        // tests/lang.rs:845     pair_transformer(pairs.iter().map(|p| &p.as_tuple()));
        //                                                             ^~~~~~~~~~~~~
    // This works now ... 
    consuming_pair_transformer(pairs.iter().map(|p| p.as_tuple()));

When looking at the example above, one might be inclined to think that serialize_owned() might want to be semantically similar to consuming_pair_transformer(...), to support the case of generated values as well.

Of course it would be better to have only one function for all of it ... I will see if I can find something useful ... .
If not, i'd clearly go for two versions of the functions, but would prefer a review of which one should be the default ... is it tuned for generated values like in the second example, or for arrays and vectors ?

from rust-url.

The builder patterns seems like 'too much' in this case. It would require the serializer to keep its own, dynamically allocated state, which is less efficient than it should be.

I don’t think this is the case. In my previous message, the internal state of Serializer is just a boolean (whether the next call needs a & separator), which can definitely be stack-allocated.

from rust-url.

Ah, so it's build on the fly. Please discard my previous comment about it needing dynamic allocation. The remaining issue I take that it is a very powerful pattern for something that effectively supports add_pair(...) only, in this example.

from rust-url.

serialize_owned can be safely disregarded. I added it as a convenience that I imagined would be nice to have, but that’s it.

That aside, I don’t understand your point in the last two comments.

from rust-url.

Maybe there is no point - in short I just liked the conciseness of the current implementation. An iterator over pairs seems like the simplest solution.

By the way: I find this example extremely interesting as it is as simple as it can get, but can teach a lot about API design in Rust.

Here we are basically talking about: How to deal with borrowed or owned values generically ?. The serialize() function is read-only, and doesn't actually care about whether the value it reads is moved or borrowed. The case were ownership is transferred to serialize() only seems interesting for generated values.

from rust-url.

Yes, good API design is hard :)

How to deal with borrowed or owned values generically ?

For just one value we have pretty good conventions: take a borrow if you don’t need to take ownership, and let deref coercions do their magic. The difficulty here is dealing with a sequence of pairs of borrowed values.

from rust-url.

Do you think a Pair trait could help ? It's surprisingly absent from std, and might be able to hide ownership.

from rust-url.

I think anything called Pair is over-engineered when we have tuples.

from rust-url.

I agree: tuples are much more elegant. But would a trait be able to hide ownership ? My feeling says: it could.

from rust-url.

I’m skeptical, but what would this trait look like?

from rust-url.

I am currently trying to put one together, still working on making the compiler happy (as you might have figured by now, I am a Rust-greenling :)!)

from rust-url.

This is how far I got - now it's telling me the following:

tests/lang.rs:902:5: 902:21 error: the trait `pair_trait_for_iteration::Pair<'_, &core::str::Str, &core::str::Str>` is not implemented for the type `&(&str, &str)` [E0277]
tests/lang.rs:902     pair_transformer([("a", "b")].iter());
                      ^~~~~~~~~~~~~~~~

Here is the code

    trait Pair<'a, A, B> {
        fn first_ref(&'a self) -> &'a A;
        fn second_ref(&'a self) -> &'a B;
    };

    struct PairOwned<A, B> {
        first: A,
        second: B,
    }

    // Only implemented for the cases we are interested in ...
    impl<'a, A, B> Pair<'a, A, B> for PairOwned<&'a A,&'a B> {
        fn first_ref(&'a self) -> &'a A {
            self.first
        }
        fn second_ref(&'a self) -> &'a B {
            self.second
        }
    }

    impl<'a, A, B> Pair<'a, A, B> for &'a(&'a A, &'a B) {
        fn first_ref(&'a self) -> &'a A {
            self.0
        }
        fn second_ref(&'a self) -> &'a B {
            self.1
        }
    }

    fn pair_transformer<'a, I, T>(pairs: I) -> String
        where   T: Pair<'a, &'a Str, &'a Str> + 'a,
                I: Iterator<Item=T> {
        let mut s = String::new();
        for pair in pairs {
            s = s
                + pair.first_ref().as_slice()
                + pair.second_ref().as_slice();
        }
        s
    }

    pair_transformer([("a", "b")].iter());

I was hoping that & will just be included in T, which would hide ownership effectively, and allow the function to work in any case, provided Pair is implemented for all the involved types. The latter is easy for two-tuples and two-arrays.

Posted the question on Stackoverflow.

from rust-url.

There is now an answer on stackoverflow.

Long story short: With the generics shown in the answer on stackoverflow, it's possible to achieve maximum generality. With the Borrowed trait, it might be possible to make it work without having an additional Pair trait.

from rust-url.

I’m confused. What change are you proposing to the form_urlencoded API? I don’t see the value of Pair over tuples, whether it’s a trait or a struct.

from rust-url.

Sorry for that.
So far we agreed that serialize_owned() should go away, which leaves serialze() to do all the work. My goal still is to find a suitable way to allow it to deal with owned tuples, as well as borrowed ones. So far I believe this might be possible with std::borrow::Borrowed, but I didn't cook anything up yet.
And no, Pair seems like overkill - introducing a new, very general trait just for URLs is nonsensical.

from rust-url.

Alright, I am just realising that I simply don't have the knowledge to make this work for borrowed and owned tuples. Eventually, I will post another question on Stackoverflow, but for today, I will let it cool down a little.
The current implementation of serialize() is fine as most flexible, even though for the future, I'd hope there are ways to allow it to take borrowed tuples as well, making it more accessible to those who feed static pairs to it.

from rust-url.

Please forget what I said recently - sometimes I have to write I am giving up, just to remind myself that I am not that kind of guy ;).

from rust-url.

url::form_urlencoded::serialize([("client_id", "bogus"),
                                 ("scope", "A B C"].iter().map(|&(a,b)| (a, b)));

To my mind, the .map() call should not be required.

Is this acceptable?

url::form_urlencoded::serialize([("client_id", "bogus"),
                                 ("scope", "A B C")].iter().cloned());

It works with the current API.

from rust-url.

In a language like Rust that aims to be "Save and Fast", I wouldn't expect anything to force you to make an unnecessary copy. What intrigues me about Rust is exactly that, as I will never make an unnecessary copy again (or allocate something unnecessarily on the heap, for that matter)

Besides that, the proposed solution I would currently prefer over the .map(...) call as it much less noisy.

from rust-url.

FWIW, .cloned() here only copies tuples of references, not the strings themselves.

from rust-url.

True, I guess performance-wise, it will be very much the same as the .map() call.

from rust-url.

I took you idea of using the Borrow trait, but with AsRef we can now abstract over &str v.s. String as well. And IntoIterator allows passing a slice or a vec directly.

The signature is now:

pub fn serialize<I, K, V>(pairs: I) -> String
where I: IntoIterator, I::Item: Borrow<(K, V)>, K: AsRef<str>, V: AsRef<str>

Thanks for your suggestions!

from rust-url.

Cheers! Thats exactly the form I use for my APIs as well.

from rust-url.