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

Data fixture #146

Merged
merged 9 commits into from Apr 24, 2017
Merged

Data fixture #146

merged 9 commits into from Apr 24, 2017

Conversation

alimanfoo
Copy link
Member

This PR addresses #138 by adding a data fixture and a test that reads the fixture data and checks against expectations, to support checks that changes to not break the ability to read previously written data.

@jakirkham
Copy link
Member

jakirkham commented Apr 24, 2017

Would it make sense to keep this data in a separate repo? Just thinking of git's generally poor performance with binary data. Also would recommend squashing before merging. Unfortunately git doesn't really do diffs with binary blobs. So squashing should cutdown on the binary content.

@alimanfoo
Copy link
Member Author

alimanfoo commented Apr 24, 2017 via email

@alimanfoo alimanfoo merged commit 6ebd0da into master Apr 24, 2017
@alimanfoo alimanfoo deleted the data-fixture branch April 24, 2017 22:54
@alimanfoo alimanfoo added this to the v2.2 milestone Apr 24, 2017
@alimanfoo alimanfoo mentioned this pull request Apr 24, 2017
@jakirkham
Copy link
Member

Sure. Mainly it is cloning the repo fresh were one gets into trouble first. Though getting the data into one blob should help. Also as it seems like the point is not to change the data, I don't imagine this should be something we need to think about too much in the future.

There are some packaging considerations when it comes to data (e.g. Python packaging of it, size considerations, and considerations for any other package artifacts), but some of these can be pushed off until later.

@alimanfoo alimanfoo mentioned this pull request Oct 24, 2017
4 tasks
@jakirkham
Copy link
Member

Should these go in MANIFEST.in? Not sure if they are required to run the test suite. Thinking about this in the context of the pending release.

@alimanfoo
Copy link
Member Author

Yes these should go in I think, so anyone can run the full test suite after install, although I should check the file sizes.

@alimanfoo alimanfoo added maintenance Work needed by a maintainer release notes done Automatically applied to PRs which have release notes. labels Nov 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Work needed by a maintainer release notes done Automatically applied to PRs which have release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants