Skip to content

Change tuple Debug impls to use builders #26913

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

Merged
merged 2 commits into from
Jul 12, 2015
Merged

Conversation

sfackler
Copy link
Member

@sfackler sfackler commented Jul 9, 2015

This does change the Debug output for 1-tuples to (foo) instead of (foo,) but I don't think it's that big of a deal.

r? @alexcrichton

@bluss
Copy link
Member

bluss commented Jul 9, 2015

Since it's using a macro, we could special case the unary tuple, so that we get the comma anyway? (1) is consistent with tuple structs, but I think it's a bit weird.

@alexcrichton
Copy link
Member

Yeah I think I'd prefer to keep the trailing comma on 1-tuples, but other than that r=me

@nagisa
Copy link
Member

nagisa commented Jul 10, 2015

Would be nice to keep Debug output as close to something that compiles. let x: (i32,) = (0); // note the missing comma does not.

@sfackler
Copy link
Member Author

Updated

@alexcrichton
Copy link
Member

In the interest of not having lingering unstable methods, could this do what @bluss mentioned and special case the 1-tuple case?

@sfackler
Copy link
Member Author

The machinery involved to properly handle the pretty case is involved enough (e.g. see fmt::builders::PadAdapter) that this is the lowest impact implementation I can see.

@bluss
Copy link
Member

bluss commented Jul 11, 2015

Yes on a closer look it's not easy, I trust what you think is best sfackler

@alexcrichton
Copy link
Member

Wouldn't it be something along the lines of:

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
    if f.alternate_flag_enabled() {
        write!(f, "({:#?},)", self.0)
    } else {
        write!(f, "({:?},)", self.0)
    }
}

@sfackler
Copy link
Member Author

That'd generate

(Foo {
    bar: hi
},)

but the builder would generate

(
    Foo {
        bar: hi
    },
)

@alexcrichton
Copy link
Member

Hm ok, seems fine then.

@bors: r+ b0ab164

@bors
Copy link
Collaborator

bors commented Jul 11, 2015

⌛ Testing commit b0ab164 with merge 0c05219...

bors added a commit that referenced this pull request Jul 11, 2015
This does change the Debug output for 1-tuples to `(foo)` instead of `(foo,)` but I don't think it's that big  of a deal.

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants