Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

src/atom/mod.rs, line 150 [r1] (raw file):
Why From<&'a String> rather than From<String>?

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 1 unresolved discussion.

src/atom/mod.rs, line 150 [r1] (raw file):
Atom::from_slice, taking a concrete &str, could be called with a &String parameter which would auto-deref: https://github.com/rust-lang/rfcs/blob/master/text/0241-deref-conversions.md

But since the parameter of From::from is generic, that coercion doesn’t work. (At least not in current Rust.) This impl helps porting existing code that relied on auto-deref, so that Atom::from_slice(&some_string) can be replaced with Atom::from(&some_string) (only renaming the method) rather than Atom::from(&*some_string) (where an explicit * deref would have to be added). I’m not sure this impl is very useful, I wouldn’t mind removing it.

From<String> would work, but it’s less useful as it consumes the string and its memory allocation.

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 1 unresolved discussion.

src/atom/mod.rs, line 150 [r1] (raw file):
Hmm, reviewable went and rewrote my question, which was "Why From<&'a String> rather than From<String>". I think this impl is confusing (at least it's confusing me!) and Atom::from(&*some_string) is more readable. This is just one instance of a general text-to-text conversion problem that I think we need a consistent story for, but that's not a problem for this PR!

Comments from the review on Reviewable.io

Review status: 3 of 5 files reviewed at latest revision, 1 unresolved discussion.

src/atom/mod.rs, line 150 [r1] (raw file):
I’ve removed this impl.

(<something> tends to be interpreted as an HTML tag by the Markdown parser.)

Comments from the review on Reviewable.io

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

@bors-servo r=asajeffrey

📌 Commit d9f3133 has been approved by asajeffrey

⌛ Testing commit d9f3133 with merge 5f0302c...

☀️ Test successful - travis