Skip to content

/rust-url

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

Add Display for HostAndPort #328

Merged
merged 4 commits into from Jun 13, 2017

Conversation

Reviewers

@SimonSapin SimonSapin

@SamWhited SamWhited

@dtolnay dtolnay

@Pistahh Pistahh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@tomecki @sigmavirus24 @tmccombs @SimonSapin @bors-servo @SamWhited @dtolnay @Pistahh
@tomecki
Copy link
Contributor

tomecki commented May 7, 2017 •
edited by larsbergstrom

Fix #300

This change is Reviewable
@tomecki
Add Display for HostAndPort
Loading status checks…
133b64e
@SamWhited

SamWhited reviewed May 9, 2017
View changes

src/host.rs Outdated
@@ -192,6 +192,15 @@ impl<'a> HostAndPort<&'a str> {
}
}

impl<S: AsRef<str>> fmt::Display for HostAndPort<S> {
fn fmt(&self, f: &mut Formatter) -> fmt::Result {
try!(self.host.fmt(f));

This comment has been minimized.

Sign in to view
Copy link
@SamWhited

SamWhited May 9, 2017

Contributor

I wonder if there's an easy way to write this all at once so that we return an Err Result and don't write anything to the formatter, or we return Ok and the formatter has the full thing? I don't think Display makes that guarantee, but it seems like a "nice to have".

This comment has been minimized.

Sign in to view
Copy link
@SimonSapin

SimonSapin May 9, 2017

Member

@SamWhited I don’t understand what you’re proposing. Could you explain some more or give an example?

In any case, it is incorrect for a Display::fmt impl to return Err other than for propagating errors that ultimately come from the Formatter. Formatting is supposed to be infallible, it’s only writing to the output stream (for example to a file on a full disk) that can fail.

This comment has been minimized.

Sign in to view
Copy link
@SamWhited

SamWhited May 9, 2017

Contributor

Yes, sorry, on rereading that was confusing.

I'm suggesting that if possible we do a single, atomic, write to the formatter. Right now we might write host: and then something bad happens and writing the port fails so we end up with a half written thing.

It's probably not actually an issue.

This comment has been minimized.

Sign in to view
Copy link
@SimonSapin

SimonSapin May 9, 2017

Member

Display impls are pretty much never "atomic" in that sense. On the contrary, they are designed to be composable like this. The only way I can think of to make on "atomic" would be to first format to a String and then write that, but that would add unnecessary costly memory allocations. A large number of them, for deeply nested Display::fmt calls.

This comment has been minimized.

Sign in to view
Copy link
@SamWhited

SamWhited May 9, 2017

Contributor

ooh yah, an allocation would be worse. LGTM then.
@Pistahh

Pistahh reviewed May 9, 2017
View changes

src/host.rs Outdated
@@ -192,6 +192,15 @@ impl<'a> HostAndPort<&'a str> {
}
}

impl<S: AsRef<str>> fmt::Display for HostAndPort<S> {
fn fmt(&self, f: &mut Formatter) -> fmt::Result {
try!(self.host.fmt(f));

This comment has been minimized.

Sign in to view
Copy link
@Pistahh

Pistahh May 9, 2017

what about using the "new" ? operator?

self.host.fmt(f)?;
self.write_str(":")?;
self.port.fmt(f)

This comment has been minimized.

Sign in to view
Copy link
@SamWhited

SamWhited May 9, 2017 •
edited

Contributor

I was about to suggest that as a nit pick :) please do (IMO)! It doesn't affect the other thing being discussed, but it is probably worth doing either way

This comment has been minimized.

Sign in to view
Copy link
@SimonSapin

SimonSapin May 9, 2017

Member

Let’s do this separately, on the whole repo: #330

This comment has been minimized.

Sign in to view
Copy link
@SimonSapin

SimonSapin May 9, 2017

Member

Since #331 has landed first, please switch to ?. Thanks!
@SimonSapin

SimonSapin reviewed May 9, 2017
View changes

tests/unit.rs Outdated
)
];

for &(ref input, result) in &data {

This comment has been minimized.

Sign in to view
Copy link
@SimonSapin

SimonSapin May 9, 2017

Member

Avoid for loops like this for unit tests. When a test fail it will show the line number for assert_eq!, but that doesn’t show which of the entries failed.

In this case I think duplicating the assert_eq! + format! line is fine, since it’s not long and there’s only two test cases. In a case when de-duplicating testing code is desirable, consider using a macro like assert_from_file_path! in this file. This makes panic messages use the line number of the macro usage.

tomecki added some commits May 9, 2017

@tomecki
Address code review issues
Loading status checks…
8f6e4f9
@tomecki
Add ? operator for Display for HostAndPort
Loading status checks…
cf77f2d
@tomecki

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

tomecki commented May 9, 2017

I addressed your comments, and added the ? operator to Display for Host. Thank you very much for your input!
@tomecki

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

tomecki commented May 9, 2017

Oops, I noticed, that ? has been added to Display for Host, added a revert commit.
@sigmavirus24

This comment has been minimized.

Sign in to view
Copy link

sigmavirus24 commented May 10, 2017

:lgtm: This looks good. Thanks for the extra tests too :)

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable
@dtolnay

dtolnay reviewed May 12, 2017
View changes

Copy link

dtolnay left a comment

Looks good to me. All of the feedback has been addressed.
@tmccombs

This comment has been minimized.

Sign in to view
Copy link
Contributor

tmccombs commented May 16, 2017

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

Comments from Reviewable
@SimonSapin

This comment has been minimized.

Sign in to view
Copy link
Member

SimonSapin commented Jun 13, 2017

Looks good.

@bors-servo r+
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Jun 13, 2017

📌 Commit a6421f6 has been approved by SimonSapin

bors-servo added a commit that referenced this pull request Jun 13, 2017

@bors-servo
Auto merge of #328 - tomecki:display_for_host_and_port, r=SimonSapin 
Add Display for HostAndPort

Fix #300

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/328)
<!-- Reviewable:end -->
Loading status checks…
4e909f6
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Jun 13, 2017

⌛️ Testing commit a6421f6 with merge 4e909f6...
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Jun 13, 2017

☀️ Test successful - status-travis
Approved by: SimonSapin
Pushing 4e909f6 to master...

@bors-servo bors-servo merged commit a6421f6 into servo:master Jun 13, 2017
3 of 4 checks passed

3 of 4 checks passed

code-review/reviewable 2 discussions left (Pistahh, SamWhited, SimonSapin)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@SimonSapin SimonSapin referenced this pull request Jun 13, 2017

Closed

Implement Display for HostAndPort #356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.