Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

PB-569 first aggregator test #353

Merged
merged 3 commits into from
Mar 31, 2020
Merged

Conversation

Robert-Steiner
Copy link
Contributor

@Robert-Steiner Robert-Steiner commented Mar 27, 2020

Summary

  • add first aggregator io test

@Robert-Steiner Robert-Steiner changed the title PB-569 first aggregator tests PB-569 first aggregator test Mar 27, 2020
Copy link
Contributor

@little-dude little-dude left a comment

Choose a reason for hiding this comment

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

That looks good to me. I left a suggestion for a simpler aggregator, but it's up to you. We should also wait for my own PR to be merged to merge this.

@Robert-Steiner Robert-Steiner force-pushed the PB-569-aggregator-tests branch 2 times, most recently from a5c606d to 28cc910 Compare March 30, 2020 19:43
@Robert-Steiner Robert-Steiner marked this pull request as ready for review March 30, 2020 19:44
@Robert-Steiner Robert-Steiner force-pushed the PB-569-aggregator-tests branch from 28cc910 to 9e700d0 Compare March 30, 2020 19:47
Copy link
Contributor

@little-dude little-dude left a comment

Choose a reason for hiding this comment

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

That looks good! I think the associated types in the ByteAggregator could be simplified a little. Also the commit history is a bit weird. To fix that easily: git fetch upstream && git reset upstream/master && git add rust/src && git commit -m 'your message' (feel free to split that into multiple commits)

@Robert-Steiner Robert-Steiner force-pushed the PB-569-aggregator-tests branch from 9326541 to 6878ae3 Compare March 31, 2020 07:18
@little-dude little-dude self-requested a review March 31, 2020 08:39
@Robert-Steiner Robert-Steiner merged commit c55ea6c into master Mar 31, 2020
@Robert-Steiner Robert-Steiner deleted the PB-569-aggregator-tests branch March 31, 2020 08:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants