Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add arbitrary trait implementation #378

Merged
merged 11 commits into from
Apr 23, 2020
Merged

Add arbitrary trait implementation #378

merged 11 commits into from
Apr 23, 2020

Conversation

kirk-baird
Copy link
Contributor

What has been done

I've added an implementation of the trait Arbitrary. It is hidden behind a feature flag arbitrary.

The implementation is pretty much the same as quickselect which implements a slightly different Arbitrary.

Additional comments

I'm not sure of the versioning so I left the version as is whether it's worth maybe a patch or minor update.

@parity-cla-bot
Copy link

It looks like @kirk-baird 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

@kirk-baird
Copy link
Contributor Author

[clabot:check]

@parity-cla-bot
Copy link

It looks like @kirk-baird signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

dvdplm
dvdplm approved these changes Apr 22, 2020
View reviewed changes
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.

Thanks!

Can you please document the feature in the README?

fixed-hash/Cargo.toml Outdated Show resolved Hide resolved
fixed-hash/src/hash.rs Outdated Show resolved Hide resolved
fixed-hash/src/hash.rs Outdated Show resolved Hide resolved
dvdplm
dvdplm reviewed Apr 22, 2020
View reviewed changes
fixed-hash/src/hash.rs Outdated Show resolved Hide resolved
ordian
ordian reviewed Apr 22, 2020
View reviewed changes
ethereum-types/Cargo.toml Outdated Show resolved Hide resolved
@kirk-baird
Copy link
Contributor Author

Thanks!

Can you please document the feature in the README?

My pleasure! It's updated.

@ordian
Copy link
Member

ordian commented Apr 22, 2020 •
edited
Loading

fixed-hash is exposed in several crates here, like primitive-types, ethbloom, do you think arbitrary feature should be propagated (re-exported) to all these crates?

@kirk-baird
Copy link
Contributor Author

fixed-hash is exposed in several crates here, like primitive-types, ethbloom, do you think arbitrary feature should be propagated (re-exported) to all these crates?

I think primitive-types is a good idea in case anyone is attempting to fuzz a struct which uses one. ethbloom less so though happy to include it for completeness.

ordian
ordian reviewed Apr 22, 2020
View reviewed changes
ethereum-types/Cargo.toml Outdated Show resolved Hide resolved
@kirk-baird @ordian
Update add ethbloom/arbitrary
a0f4828 
Co-Authored-By: Andronik Ordian <[email protected]>
@kirk-baird kirk-baird mentioned this pull request Apr 22, 2020
Merged
dvdplm
dvdplm suggested changes Apr 22, 2020
View reviewed changes
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.

Some more nitpicking.

uint/README.md Outdated Show resolved Hide resolved
fixed-hash/src/hash.rs Outdated Show resolved Hide resolved
@dvdplm
Copy link
Contributor

dvdplm commented Apr 22, 2020

I'm not sure of the versioning so I left the version as is whether it's worth maybe a patch or minor update.

This is a patch update I'd say. What we usually do is add unpublished changes to the CHANGELOG under the "Unreleased" header. When the time comes for publication we'll bump the version and fix up the change log.

Speaking of releases: do you have an immediate need for this?

kirk-baird and others added 2 commits April 23, 2020 10:52
@kirk-baird
Copy link
Contributor Author

This is a patch update I'd say. What we usually do is add unpublished changes to the CHANGELOG under the "Unreleased" header. When the time comes for publication we'll bump the version and fix up the change log.

Do I need to make any changes to the CHANGELOG or will you guys handle that?

Speaking of releases: do you have an immediate need for this?

It's not mission critical, I'm currently using a patch to point to my branch.

dvdplm
dvdplm approved these changes Apr 23, 2020
View reviewed changes
fixed-hash/src/hash.rs Outdated Show resolved Hide resolved
fixed-hash/src/hash.rs Outdated Show resolved Hide resolved
@dvdplm dvdplm removed the A5-grumble label Apr 23, 2020
@ordian
Copy link
Member

ordian commented Apr 23, 2020

Do I need to make any changes to the CHANGELOG or will you guys handle that?

We can handle that, no worries.

@ordian ordian merged commit 6bb24f2 into paritytech:master Apr 23, 2020
ordian added a commit that referenced this pull request Apr 23, 2020
@ordian
Merge branch 'master' into ao-upgrade-rocksdb
36f799f 
* master:
  Fix limit prefix delete case (#368)
  Add arbitrary trait implementation (#378)
@ordian ordian mentioned this pull request Apr 27, 2020
Merged
ordian added a commit that referenced this pull request May 5, 2020
@ordian
Merge branch 'master' into ao-type-metadata
dc92ac3 
* master: (56 commits)
  primitive-types: add no_std support for serde feature (#385)
  Add Rocksdb Secondary Instance Api (#384)
  kvdb-rocksdb: update rocksdb to 0.14 (#379)
  prepare releases for a few crates (#382)
  uint: fix UB in uint::from_big_endian (#381)
  Fix limit prefix delete case (#368)
  Add arbitrary trait implementation (#378)
  kvdb-rocksdb: optimize and rename iter_from_prefix  (#365)
  bump parity-util-mem (#376)
  parity-util-mem: fix for windows (#375)
  keccak-hash: fix bench and add one for range (#372)
  [parity-crypto] Release 0.6.1 (#373)
  keccak-hash: bump version to 0.5.1 (#371)
  keccak-hash: add keccak256_range and keccak512_range functions (#370)
  Allow pubkey recovery for all-zero messages (#369)
  Delete by prefix operator in kvdb (#360)
  kvdb: no overlay (#313)
  Ban duplicates of parity-uil-mem from being linked into the same program (#363)
  Use correct license ID (#362)
  Memtest example for Rocksdb (#349)
  ...
@paulhauner paulhauner mentioned this pull request Oct 12, 2021
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ordian ordian ordian approved these changes

@dvdplm dvdplm dvdplm approved these changes

Assignees
No one assigned
Labels
A8-looksgood
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kirk-baird @parity-cla-bot @ordian @dvdplm