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

`form_urlencoded::serialize(...)` handles owned as well as borrowed pairs #82

Merged
merged 1 commit into from Apr 30, 2015

Conversation

@Byron
Copy link
Contributor

Byron commented Feb 24, 2015

This is the result of a long path full of (my own) trial and error on how to allow serialize() not to be hardcoded to owned tuples, and increase usability.

I have added some tests which prove it now works both ways, without altering the existing API in any way (i.e. this is not a breaking change).

Even though discussed in #81, I didn't removed serialize_owned(), as it's possibly breaking and I would want to leave that to the superior judgement of the maintainers.

@SimonSapin
Copy link
Member

SimonSapin commented Feb 24, 2015

the superior judgement of the maintainers.

These are strong words, when the maintainers often feel like this :)

Regardless, I’m a bit reluctant to add complexity to this signature, when it seems the only reason borrowed tuples are around in the first place is that [T; N] doesn’t implement IntoIterator. See also #81 (comment)

@Byron
Copy link
Contributor Author

Byron commented Feb 24, 2015

Well, at least I suspect the maintainers have less 'no idea of what they are doing' than me :).

If until Rust beta, [T; N] should implement IntoIterator (and I am sceptic this will happen), my change indeed shouldn't be necessary.
One could wait with the merge until then.

Personally I think the complexity in the signature is just what Rust is, and should be something we all get more and more accustomed to. Also I prefer adding complexity to the implementation to keep it light and simple on the caller's end. The needs of the many outweigh the needs of the few :) !

#81 comment will be addressed there.

@SimonSapin SimonSapin merged commit 503f52f into servo:master Apr 30, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.