Implement Debug for public types

[danburkert]

Hey @cmbrandenburg, can you share what the motivation is for adding these impls? I'm not against adding Debug to these types, but since the default derived implementation would mostly be ptr addrs, perhaps it's better just to do something like

impl Debug for RoTransaction {
    fn fmt(&self, f: &mut Formatter) -> Result<(), Error> {
        write!(f, "RoTransaction")
    }
}

[cmbrandenburg]

My motive is laziness—i.e., taking the easiest path possible to having Debug implemented. I don't care what info the output contains and am fine with someone else customizing the impl to obviate pointer values.

[danburkert]

Yah I think it's probably best not to print the internal details unless there's a compelling reason.

[cmbrandenburg]

I took a deeper look at this and now I disagree. From the Rust Standard Library documentation (for the std::fmt module):

fmt::Debug implementations should be implemented for all public types. Output will typically represent the internal state as faithfully as possible. The purpose of the Debug trait is to facilitate debugging Rust code. In most cases, using #[derive(Debug)] is sufficient and recommended.

From this, I draw two conclusions:

1. Having a Debug impl for these public types is better than having no Debug impl.

2. Customizing the Debug impl to omit pointer fields—however obscure—is less faithful than including those fields. However, omitting the PhantomData fields is okay because they include no run-time data.

In my opinion, customizing the Debug impl merely to omit PhantomData fields is overkill. Also, any custom impl for a struct is vulnerable to someone adding a field to the struct without remembering to update the custom impl, which would cause the impl to be incomplete. Custom impls just aren't worth it without good reason—which I think is what the Rust doc is suggesting.

For what it's worth, I implemented full omission but did so in a separate branch, here, not to be included as part of this pull request.

I think this pull request should be merged as is. If you really feel strongly about omitting part of the internal representation, then please feel free to fetch my other branch and merge it in.

[danburkert]

I agree with the premise that the types should have Debug impls. However, Debug implementations frequently leave out information if it's not expected to be useful. For instance, the Debug impl for Box doesn't include the heap pointer, nor does Vec include its heap pointer.

https://github.com/cmbrandenburg/lmdb-rs-1/commit/5f05b5892ad88c91a775a541e5b360e1182016d0 looks good to me, but I think it would be a little simpler if it imported std::fmt and used fmt::Result instead of fully qualifying everything.

[danburkert]

After taking a second look, I don't think the op or next_op fields on the Iterator type are even worth printing. Those are very much internal implementation details.

[cmbrandenburg]

@danburkert, I've made the changes you've suggested and rebased down to a single commit.

[danburkert]

Thanks, looks good, just a small nit remaining.

[cmbrandenburg]

Updated and rebased.

[danburkert]

Thanks! Released in 0.7.0.