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.

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

Add drain method to Storage #273

Merged
merged 1 commit into from
Sep 29, 2017
Merged

Add drain method to Storage #273

merged 1 commit into from
Sep 29, 2017

Conversation

torkleyy
Copy link
Member

Fixes #264

@torkleyy
Copy link
Member Author

r? @malleusinferni @Aceeri

@torkleyy
Copy link
Member Author

I'm not sure why coverage goes down.. does it really want me to test drain for every type of storage?

@WaDelma
Copy link
Member

WaDelma commented Sep 24, 2017

You should be able to make coveralls to show the file and which lines are covered by test.
Login on coveralls using github, click file and it should let you fill the where the sources are (which is src for us=

@torkleyy
Copy link
Member Author

The problem is that the coverage is inaccurate. E.g. I had to test derived traits, but that doesn't make any sense. I ran the coverage locally and it was not obvious what test I should add, see Gitter.

@malleusinferni
Copy link
Contributor

Looks good to me. I tried it out with a few reduced test cases and it does exactly what I was asking for. Thanks @torkleyy!

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

this is pretty cool!
ready to merge?

@torkleyy
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Sep 29, 2017
273: Add `drain` method to `Storage` r=torkleyy a=torkleyy

Fixes #264
@bors
Copy link
Contributor

bors bot commented Sep 29, 2017

Build succeeded

@bors bors bot merged commit c681f27 into amethyst:master Sep 29, 2017
@torkleyy torkleyy deleted the drain branch September 29, 2017 11:26
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