Skip to content

[fixed-hash] Add deprecations in favor of the upcoming 0.3 major version #72

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 3 commits into from
Oct 28, 2018

Conversation

Robbepop
Copy link

@Robbepop Robbepop commented Oct 25, 2018

NOTE

  • The failing CI tests are a result of this Rust compiler bug.
  • Please merge so that we can continue work on the other PR asap.

Description

Pre 0.3 deprecations for fixed-hash crate as suggested by @dvdplm .

Note that deprecation of trait implementations do not work in general for Rust.
Note sure how to handle those, especially for the Deref and DerefMut trait implementations.

Also note that there will be some more trait implementation deprecations coming with the future 0.3 release. Especially for the conversions that have no error handling so far.

@parity-cla-bot
Copy link

It looks like @Robbepop signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@Robbepop Robbepop changed the title Add deprecations in favor of the upcoming 0.3 major version [fixed-hash] Add deprecations in favor of the upcoming 0.3 major version Oct 25, 2018
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, exactly what I was hoping for. Have some change suggestions for wording and a question.

@@ -7,6 +7,10 @@
// except according to those terms.

/// Return `s` without the `0x` at the beginning of it, if any.
#[deprecated(
since = "0.2.5",
note = "implement this 3-liner yourself instead"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

…but let's be Enterprise Ready® and say "out of scope of fixed-hash".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅

@@ -104,26 +124,42 @@ macro_rules! construct_hash {

#[inline]
/// Assign self to be of the same value as a slice of bytes of length `len()`.
#[deprecated(
since = "0.2.5",
note = "unconventional API, will have a replacement in version 0.3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we state what the replacement is? So: unconventional API, replaced by … … in 0.3?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, would be better!

pub fn clone_from_slice(&mut self, src: &[u8]) -> usize {
let min = ::core::cmp::min($size, src.len());
self.0[..min].copy_from_slice(&src[..min]);
min
}

/// Convert a slice of bytes of length `len()` to an instance of this type.
#[deprecated(
since = "0.2.5",
note = "unconventional API, will have a replacement in version 0.3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

pub fn from_slice(src: &[u8]) -> Self {
let mut r = Self::new();
r.clone_from_slice(src);
r
}

/// Copy the data of this object into some mutable slice of length `len()`.
#[deprecated(
since = "0.2.5",
note = "simply use std slice API instead"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the "simply": use the std::slice API instead.

@@ -309,6 +350,10 @@ macro_rules! construct_hash {
fn default() -> Self { $from::new() }
}

#[deprecated(
since = "0.2.5",
note = "big-endian and little-endian confusion"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe will be removed because not big-/little-endian aware?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds better, thanks!

@@ -322,6 +367,10 @@ macro_rules! construct_hash {
}
}

#[deprecated(
since = "0.2.5",
note = "no proper error handling possible with out-of-bounds inputs"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remind me, what is the replacement for this in 0.3? I could be wrong but I think this one is fairly often used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no concrete replacement API in mind for this but I would be O.K. with replacing this by a function:

fn from_slice(slice: &[u8]) -> $from {
    assert_eq!(
        $from::len_bytes(), slice.len(),
        "[fixed-hash] error: cannot create a fixed-hash from a byte slice with different length"
    );
    $from::from(std::mem::transmute::<&[u8], &[u8; $n_bytes]>(slice))
}

Of course I really hope for not having to use mem::transmute.

@Robbepop
Copy link
Author

@dvdplm What has to be done in order to merge this?

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dvdplm dvdplm requested a review from debris October 25, 2018 20:07
Copy link

@lexfrl lexfrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@Robbepop Robbepop merged commit 9fdf2f6 into master Oct 28, 2018
@Robbepop Robbepop deleted the pre-0.3-deprecations branch October 28, 2018 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants