Skip to content

Upgrade depenencies#236

Closed
Demi-Marie wants to merge 1 commit into
masterfrom
upgrade-deps
Closed

Upgrade depenencies#236
Demi-Marie wants to merge 1 commit into
masterfrom
upgrade-deps

Conversation

@Demi-Marie
Copy link
Copy Markdown
Contributor

No description provided.

@parity-cla-bot
Copy link
Copy Markdown

It looks like @demimarie-parity hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@dvdplm
Copy link
Copy Markdown
Contributor

dvdplm commented Oct 9, 2019

A lot of funny looking lifetime annotations appeared here, why is that? And while explicit versions down to the patch level is fine I am not sure they are strictly needed, can you elaborate a bit on what motivates these changes?

@niklasad1
Copy link
Copy Markdown
Contributor

@dvdplm I think it fixes a lint/warning part of the rust_2018_idioms which is warning: hidden lifetime parameters in types are deprecated

Comment thread fixed-hash/src/hash.rs
}

/// Create a new hash with cryptographically random content.
pub fn random() -> Self {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why remove? It is a breaking change

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

rustc warned about it being unused.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How can cargo fix know that a pub item is not used?

Copy link
Copy Markdown
Contributor

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

IMHO, this PR does more than upgrading dependencies.

I would prefer if you could create separate PRs for those, such as enabling anonymous lifetimes, removing fixed_hash::random, replacing mem::uninitialized() with MaybeUninit and so on.



extern crate parity_crypto;
use parity_crypto;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not required.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. This was done automatically.

Comment thread kvdb-rocksdb/src/lib.rs
#[cfg(test)]
mod tests {
extern crate tempdir;
use tempdir;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not required.

Just remove the use self::tempdir below and just use tempdir::

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is indeed not required. The changes to the Rust code were done automatically by cargo fix.

@Demi-Marie
Copy link
Copy Markdown
Contributor Author

A lot of funny looking lifetime annotations appeared here, why is that? And while explicit versions down to the patch level is fine I am not sure they are strictly needed, can you elaborate a bit on what motivates these changes?

The lifetime annotations were generated by cargo fix. The explicit versioning was done by cargo upgrade. It isn’t practical to do these codebase-wide changes without automation.

@Demi-Marie Demi-Marie closed this Oct 9, 2019
@Demi-Marie
Copy link
Copy Markdown
Contributor Author

Closing in favor of #237, #238, and #239.

@Demi-Marie Demi-Marie deleted the upgrade-deps branch October 9, 2019 15:23
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.

5 participants