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

Fix #305 Implement Debug for many types #335

Merged
merged 6 commits into from Jun 13, 2017

Conversation

Projects
None yet
6 participants
@imor
Copy link
Contributor

imor commented May 10, 2017

For most types it was a simple matter of adding #[derive(Debug)] but
ParseOptions needed a manual implementation because the type of
log_syntax_violation is Option<&'a Fn(&'static str)> and Fn doesn't
implement Debug. log_syntax_violation is formatted as Some(Fn(&'static
str)) or None depending upon its value.


This change is Reviewable

imor added some commits May 10, 2017

Fix #305 Implement Debug for many types
For most types it was a simple matter of adding #[derive(Debug)] but
ParseOptions needed a manual implementation because the type of
log_syntax_violation is Option<&'a Fn(&'static str)> and Fn doesn't
implement Debug. log_syntax_violation is formatter as Some(Fn(&'static
str)) or None depending upon its value.
Fix failing build on travis-ci
Implemented Debug for EncodingOverride as EncodingRef doesn't implement
Debug. Field 'encoding' is formatted as Some(EncodingOverride) or None
based on its value.
@saghm

This comment has been minimized.

Copy link

saghm commented May 11, 2017

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.


src/encoding.rs, line 98 at r1 (raw file):

        write!(f, "EncodingOverride {{ encoding: ")?;
        match self.encoding {
            Some(_) => write!(f, "Some(EncodingOverride) }}"),

Could this instead call name on the wrapped encoding ref? I think it might be useful to be able to see the the type of the encoding if it's present.


Comments from Reviewable

@saghm

This comment has been minimized.

Copy link

saghm commented May 11, 2017

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


Comments from Reviewable

@@ -213,6 +213,16 @@ impl<'a> ParseOptions<'a> {
}
}

impl<'a> Debug for ParseOptions<'a> {

This comment has been minimized.

@turnage

turnage May 11, 2017

The body of this function can just be

(self.base_url, self.encoding_override).fmt(f)

Edit: I've never done a github review before so if this comment blocks or something feel free to close it with a garbage comment.

This comment has been minimized.

@imor

imor May 12, 2017

Author Contributor

What about the log_syntax_violation field? Shouldn't it be included? And as it includes a Fn in the type, how is it usually formatted?

@@ -89,9 +90,19 @@ impl EncodingOverride {
}
}

#[cfg(feature = "query_encoding")]
impl Debug for EncodingOverride {

This comment has been minimized.

@turnage

turnage May 11, 2017

You can just derive Debug on the structs compiled for each feature.

This comment has been minimized.

@imor

imor May 12, 2017

Author Contributor

Can't derive Debug in this case because encoding field has type Option<EncodingRef> and EncodingRef trait isn't Debug

@@ -77,7 +77,7 @@ macro_rules! define_encode_set {
}

/// This encode set is used for the path of cannot-be-a-base URLs.
#[derive(Copy, Clone)]
#[derive(Copy, Clone, Debug)]
#[allow(non_camel_case_types)]
pub struct SIMPLE_ENCODE_SET;

This comment has been minimized.

@turnage

turnage May 11, 2017

You need to derive Debug in the encoding set macro to get the rest of *_ENCODE_SET types.

This comment has been minimized.

@imor

imor May 12, 2017

Author Contributor

Right, will do.

This comment has been minimized.

@imor

imor May 12, 2017

Author Contributor

Done.

@dtolnay

This comment has been minimized.

Copy link

dtolnay commented May 11, 2017

The write! approach isn't a great way to do Debug implementations because it doesn't respect the various flags on the Formatter.

use std::fmt::{self, Debug};

struct Good { imor: i32 }
impl Debug for Good {
    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
        formatter.debug_struct("Good")
            .field("imor", &self.imor)
            .finish()
    }
}

struct Bad { imor: i32 }
impl Debug for Bad {
    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
        write!(formatter, "Bad {{ imor: {:?} }}", self.imor)
    }
}

fn main() {
    let good = Good { imor: 0 };
    let bad = Bad { imor: 0 };
    println!("default: {:?}", good);
    println!("indented: {:#?}", good);
    println!();
    println!("default: {:?}", bad);
    println!("indented: {:#?}", bad);
}
default: Good { imor: 0 }
indented: Good {
    imor: 0
}

default: Bad { imor: 0 }
indented: Bad { imor: 0 }

Is there a way to do these only using derive(Debug) and debug_struct?

@imor

This comment has been minimized.

Copy link
Contributor Author

imor commented May 12, 2017

Review status: 5 of 6 files reviewed at latest revision, 4 unresolved discussions.


src/encoding.rs, line 98 at r1 (raw file):

Previously, saghm (Saghm Rossi) wrote…

Could this instead call name on the wrapped encoding ref? I think it might be useful to be able to see the the type of the encoding if it's present.

Done.


Comments from Reviewable

@SimonSapin

This comment has been minimized.

Copy link
Member

SimonSapin commented Jun 13, 2017

Thanks!

@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jun 13, 2017

📌 Commit 81fdb30 has been approved by SimonSapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jun 13, 2017

⌛️ Testing commit 81fdb30 with merge 4083375...

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

Auto merge of #335 - imor:impl-debug, r=SimonSapin
Fix #305 Implement Debug for many types

For most types it was a simple matter of adding #[derive(Debug)] but
ParseOptions needed a manual implementation because the type of
log_syntax_violation is Option<&'a Fn(&'static str)> and Fn doesn't
implement Debug. log_syntax_violation is formatted as Some(Fn(&'static
str)) or None depending upon its value.

<!-- 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/335)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jun 13, 2017

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

@bors-servo bors-servo merged commit 81fdb30 into servo:master Jun 13, 2017

3 of 4 checks passed

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

@imor imor deleted the imor:impl-debug branch Jun 22, 2017

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.