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

Stabilize debug builders #24847

Merged
merged 1 commit into from May 23, 2015

Conversation

Projects
None yet
7 participants
@sfackler
Copy link
Member

sfackler commented Apr 26, 2015

The debug_builders feature is up for 1.1 stabilization in #24028. This commit stabilizes the API as-is with no changes.

Some nits that @alexcrichton mentioned that may be worth discussing now if anyone cares:

  • Should debug_tuple_struct and DebugTupleStruct be used instead of debug_tuple and DebugTuple? It's more typing but is a technically more correct name.
  • DebugStruct and DebugTuple have field methods while DebugSet, DebugMap and DebugList have entry methods. Should we switch those to something else for consistency?

cc @alexcrichton @aturon

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Apr 26, 2015

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Apr 26, 2015

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson Apr 26, 2015

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 30, 2015

☔️ The latest upstream changes (presumably #24967) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 30, 2015

We got a chance to talk about this today, and the general feeling here is that this stabilization feels a little rushed. I'm curious, but do you know if there's much usage of this feature today? I was personally unaware of many users of this feature or your debug-builders crate.

Overall these APIs also seem like they haven't been subjected to the scrutiny one might expect from a stable API (from the community at large, not necessarily just us per se). In terms of stabilizing new APIs in the standard library we've toyed with the idea of a "final comment period"-like segment of time where users of Rust can browse soon-to-be-stable APIs, but this would take some infrastructure investment to see it realized.

To be clear I don't think the 1.1 window has passed for this feature once we release 1.0, I'm fine cherry picking a change such as this back into beta.

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Apr 30, 2015

I'm not aware of a ton of consumers of the API - explicit Debug impls appear to be pretty rare: https://github.com/search?l=rust&p=3&q=%22Debug+for%22&ref=searchresults&type=Code&utf8=%E2%9C%93

Every derived Debug implementation is a consumer, though :)

I think a "final comment period" style thing is a good idea, especially for APIs like this that aren't incredibly heavily used.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Apr 30, 2015

I used this API recently, and I wanted a way to display one of my fields in hex. Can we find a simple composable way to do that? I sort of wanted something like this to work: .field("flag", &tag as &fmt::LowerHex).

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Apr 30, 2015

The most straightforward way to deal with that right now would be something like

struct DebugHex<T>(T);

impl<T: LowerHex> Debug for DebugHex<T> {
    fn fmt(&self, fmt: &mut Formatter) -> Result {
        self.0.fmt(fmt)
    }
}

...
    .field("flag", &DebugHex(&tag))
...

I'm not sure if there's a way to abstract over Debug, LowerHex etc in a way that would work with your syntax.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 30, 2015

@sfackler would you be ok making a post on internals to see if others have comments? (like @bluss!)

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented May 8, 2015

@bluss Actually realized that my original suggestion is both more verbose than it needs to be and not quite right. We don't want to directly forward the Formatter object into LowerHex's fmt method since we wouldn't want to pass along extra formatter flags etc like # since it'll change behavior. A more robust way would be to use format_args! which lets you specify exactly which formatter trait and flags you want to use:

....
    .field("flag", &format_args!("{:x}", self.tag))
....
@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented May 8, 2015

Excellent, that already works!

@sfackler sfackler force-pushed the sfackler:debug-builders-stability branch from f18282b to e161d5c May 20, 2015

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented May 20, 2015

Rebased and shifted to stabilize for 1.2.0 instead of 1.1.0.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented May 21, 2015

@bors: r+

Congrats, and thanks, @sfackler!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 21, 2015

📌 Commit e161d5c has been approved by aturon

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 21, 2015

⌛️ Testing commit e161d5c with merge 258907d...

bors added a commit that referenced this pull request May 21, 2015

Auto merge of #24847 - sfackler:debug-builders-stability, r=aturon
The `debug_builders` feature is up for 1.1 stabilization in #24028. This commit stabilizes the API as-is with no changes.

Some nits that @alexcrichton mentioned that may be worth discussing now if anyone cares:

* Should `debug_tuple_struct` and `DebugTupleStruct` be used instead of `debug_tuple` and `DebugTuple`? It's more typing but is a technically more correct name.
* `DebugStruct` and `DebugTuple` have `field` methods while `DebugSet`, `DebugMap` and `DebugList` have `entry` methods. Should we switch those to something else for consistency?

cc @alexcrichton @aturon
@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 21, 2015

⛄️ The build was interrupted to prioritize another pull request.

bors added a commit that referenced this pull request May 21, 2015

Auto merge of #24847 - sfackler:debug-builders-stability, r=aturon
The `debug_builders` feature is up for 1.1 stabilization in #24028. This commit stabilizes the API as-is with no changes.

Some nits that @alexcrichton mentioned that may be worth discussing now if anyone cares:

* Should `debug_tuple_struct` and `DebugTupleStruct` be used instead of `debug_tuple` and `DebugTuple`? It's more typing but is a technically more correct name.
* `DebugStruct` and `DebugTuple` have `field` methods while `DebugSet`, `DebugMap` and `DebugList` have `entry` methods. Should we switch those to something else for consistency?

cc @alexcrichton @aturon
@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 21, 2015

⌛️ Testing commit e161d5c with merge 806e61f...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 22, 2015

💔 Test failed - auto-mac-32-opt

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented May 22, 2015

@bors retry

Manishearth added a commit to Manishearth/rust that referenced this pull request May 22, 2015

Rollup merge of rust-lang#24847 - sfackler:debug-builders-stability, …
…r=aturon

The `debug_builders` feature is up for 1.1 stabilization in rust-lang#24028. This commit stabilizes the API as-is with no changes.

Some nits that @alexcrichton mentioned that may be worth discussing now if anyone cares:

* Should `debug_tuple_struct` and `DebugTupleStruct` be used instead of `debug_tuple` and `DebugTuple`? It's more typing but is a technically more correct name.
* `DebugStruct` and `DebugTuple` have `field` methods while `DebugSet`, `DebugMap` and `DebugList` have `entry` methods. Should we switch those to something else for consistency?

cc @alexcrichton @aturon

bors added a commit that referenced this pull request May 23, 2015

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 23, 2015

⌛️ Testing commit e161d5c with merge 8bc80ba...

bors added a commit that referenced this pull request May 23, 2015

Auto merge of #24847 - sfackler:debug-builders-stability, r=aturon
The `debug_builders` feature is up for 1.1 stabilization in #24028. This commit stabilizes the API as-is with no changes.

Some nits that @alexcrichton mentioned that may be worth discussing now if anyone cares:

* Should `debug_tuple_struct` and `DebugTupleStruct` be used instead of `debug_tuple` and `DebugTuple`? It's more typing but is a technically more correct name.
* `DebugStruct` and `DebugTuple` have `field` methods while `DebugSet`, `DebugMap` and `DebugList` have `entry` methods. Should we switch those to something else for consistency?

cc @alexcrichton @aturon

@bors bors merged commit e161d5c into rust-lang:master May 23, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented May 26, 2015

🌴

@sfackler sfackler deleted the sfackler:debug-builders-stability branch Nov 26, 2016

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.