Skip to content

[code hygiene] remove deprecated rustc-serialize#107

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
savil:remove-rustc-serialize
Jul 28, 2018
Merged

[code hygiene] remove deprecated rustc-serialize#107
apoelstra merged 1 commit intorust-bitcoin:masterfrom
savil:remove-rustc-serialize

Conversation

@savil
Copy link
Contributor

@savil savil commented Jul 24, 2018

Addresses #96.

Turns out it was being used for hex encoding/decoding, so replaced that with the hex crate.

i chose to import the decode method as:

use hex::decode as hex_decode

so that it is clear to the reader what is being decoded when it is called. "decode" is such a generic sounding function name that it would get confusing otherwise.

@dongcarl
Copy link
Member

@savil As per IRC discussion, please pin the hex dependency to a commit hash, you can see how to do so in #108

@dongcarl
Copy link
Member

dongcarl commented Jul 24, 2018

Otherwise: ACK

@savil
Copy link
Contributor Author

savil commented Jul 24, 2018

fixed the hex-dependency to a specific commit-hash, as requested. I don't have strong opinions on this issue.

in IRC, @dongcarl said that its more reliable that version-numbers since some package managers can change the code despite the version number being the same.

I will note that in Cargo.toml, other dependencies all use version numbers. shrugs

@TheBlueMatt
Copy link
Member

TheBlueMatt commented Jul 24, 2018

Pasting dependency discussion from IRC:

<BlueMatt> andytoshi: heh, I mean I guess pinning a dep for hex is ok, though given its use in like only one place (and only serialize!) in non-test, you could really make it a test-only dep and add the written out hex encoder in the one place its used
<BlueMatt> (which would only be a -1 line +4 lines diff
<BlueMatt> oh, except for one place thats used in a serde deserializer, but we can make that optional on the basis of wehether you're building with serde support or not (which I think we said we agreed should be optional)

It may be out of scope to do the serde-optional thing in this PR, but would be super, super nice to make that happen IMO.

@savil
Copy link
Contributor Author

savil commented Jul 25, 2018

I'll look into making it test-only tomorrow

@apoelstra
Copy link
Member

I think we should leave the hex dependency for now. I'll remove hex later when I move to serde 1.0 and make sure everything that should be optional is optional.

@apoelstra
Copy link
Member

"Later" still meaning before next release.

Addresses #96.

Turns out it was being used for hex encoding/decoding, so replaced that with the `hex` crate.

i chose to import the `decode` method as:
```
use hex::decode as hex_decode
```

so that it is clear to the reader what is being decoded when it is called. "decode" is such a generic sounding function name that it would get confusing otherwise.
@TheBlueMatt
Copy link
Member

Yea, that seems fine to me, no reason to rush it, especially if its a git commit dep for now.

@savil
Copy link
Contributor Author

savil commented Jul 26, 2018

cool - assuming nothing more is required for this PR. Let me know if any more changes are requested.

@apoelstra apoelstra merged commit cdb7ca8 into rust-bitcoin:master Jul 28, 2018
Tibo-lg pushed a commit to p2pderivatives/rust-bitcoin that referenced this pull request Sep 1, 2020
yancyribbens pushed a commit to yancyribbens/rust-bitcoin that referenced this pull request Mar 23, 2024
PastaPastaPasta pushed a commit to PastaPastaPasta/rust-dashcore that referenced this pull request Aug 21, 2025
* fixes

* update rust version

* update rust

* cleanup
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.

4 participants