Fix deserialization

[A-Manning]

Fixes #189.

The changes in src/display.rs and src/iter.rs were necessary, since using serde_json in tests brings an impl PartialEq<serde_json::Value> for usize into context, which makes some code in the aforementioned tests ambiguous without explicitly specifying types

[apoelstra]

I think the version of serde_json in Cargo.lock (and Cargo.toml) is too old.

[A-Manning]

I think the version of serde_json in Cargo.lock (and Cargo.toml) is too old.

Fixed

[tcharding]

ACK 594983c

[tcharding]

To get past CI you'll need to run 'just update-lock-files' and force push please.

[tcharding]

I had play with your tests, perhaps this is cleaner?

#[cfg(test)]
mod test {
    use serde_json::value::Serializer;

    use super::*;

    #[test]
    fn serialize_lower_roundtrip() {
        let bytes: [u8; 4] = [0xdeu8, 0xad, 0xbe, 0xef];

        let serialized = serialize_lower(&bytes, Serializer).expect("serialize");
        let deserialized: [u8; 4] = deserialize(serialized).expect("deserialize");

        assert_eq!(deserialized, bytes);
    }

    #[test]
    fn serialize_upper_roundtrip() {
        let bytes: [u8; 4] = [0xdeu8, 0xad, 0xbe, 0xef];

        let serialized = serialize_upper(&bytes, Serializer).expect("serialize");
        let deserialized: [u8; 4] = deserialize(serialized).expect("deserialize");

        assert_eq!(deserialized, bytes);
    }
}

[tcharding]

And thanks for the contribution, and for finding our (my) bugs!

[A-Manning]

To get past CI you'll need to run 'just update-lock-files' and force push please.

Pushed

[A-Manning]

I had play with your tests, perhaps this is cleaner?

IMO it is cleaner for tests to return Result<(), SomeError> than to have .expect("some error message") throughout. I also prefer to avoid wildcard imports where possible. I'd prefer to keep the tests as-is

[apoelstra]

ACK e1fa1d8; successfully ran local tests

[tcharding]

I had play with your tests, perhaps this is cleaner?

IMO it is cleaner for tests to return Result<(), SomeError> than to have .expect("some error message") throughout. I also prefer to avoid wildcard imports where possible. I'd prefer to keep the tests as-is

Sure, I'm not that bothered but for the record neither of these is done anywhere else in this codebase (or the rust-bitocin org) as far as I'm aware.

[apoelstra]

Huh. I actually didn't notice the Result return type here, nor did I know that it was allowed (since Rust 2018, apparently). I kinda prefer it.

[apoelstra]

As for avoiding wildcards, in the specific case of super::* in inline modules, I don't mind them, because any symbols it exports you know are defined in the same file that you're already looking at.

But agreed in general it's better to avoid them.

I'm gonna go ahead and merge this, and if we want to do some broader unit test refactoring effort, we can do that separately.

[tcharding]

I also didn't know it was allowed. Interestingly GitHub does not pick up the mention, here it is: rust-bitcoin/rust-bitcoin#4882

[tcharding]

Backported to 0.3.x in #201 and to 0.2.x in #202