-
Notifications
You must be signed in to change notification settings - Fork 182
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
Added address reference to CLI output #556
Conversation
src/cli/serve.rs
Outdated
@@ -88,7 +88,14 @@ fn show_start_message(bind_address: IpAddr, port: u16, color: ColorChoice) -> io | |||
write!(&mut buffer, "Visit ")?; | |||
|
|||
buffer.set_color(&green)?; | |||
write!(&mut buffer, "http://localhost:{}/", port)?; | |||
if bind_address.is_loopback() { | |||
write!(&mut buffer, "http://localhost:")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same loopback check is done to display the address earlier. You would be better off storing the address in a variable that is used for both displays IMO.
@Kampfkarren Thanks for the feedback, all sorted! |
src/cli/serve.rs
Outdated
@@ -66,6 +66,8 @@ fn show_start_message(bind_address: IpAddr, port: u16, color: ColorChoice) -> io | |||
|
|||
let writer = BufferWriter::stdout(color); | |||
let mut buffer = writer.buffer(); | |||
|
|||
let address_string = if bind_address.is_loopback() {"localhost"} else {stringify!(bind_address)}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stringify! is definitely not what you want here, it's just going to return "bind_address" itself.
Change "localhost" to "localhost".to_owned(), and the other side to bind_address.to_string().
Then run cargo fmt, since this is going to fail that test 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I tried doing that before but my rust linter was saying that .to_string() wasn't a function in the struct.
Turns out my linter is slow and didn't catch up when I was trying to debug it! So I just tried using stringify!() and the problem went away - forgot that stringify!() does not do what I was trying to.
Anyway, sorted!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, thanks for submitting a PR! I just got back from vacation, submitted a little feedback and approved the CI workflows to run.
src/cli/serve.rs
Outdated
buffer.set_color(&green)?; | ||
write!(&mut buffer, "http://localhost:{}/", port)?; | ||
write!(&mut buffer, "http://{}:", address_string)?; | ||
|
||
buffer.set_color(&green)?; | ||
write!(&mut buffer, "{}/", port)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge these calls together. You can pass multiple arguments into the same format string, something like this should work:
write!(&mut buffer, "http://{}:{}/", address_string, port)?;
* Added address reference to CLI output * Stored loopback check address as a variable * Changed other loopback references to the new variable * Fixed mistake on address_string variable * Merge write calls Co-authored-by: Lucien Greathouse <me@lpghatguy.com>
This pull request just fixes the CLI bug on #555
Here is a screenshot of the changes on the output