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

Re-organize the fixture filler helper tools #1219

Merged

Conversation

pipermerriam
Copy link
Member

@pipermerriam pipermerriam commented Aug 28, 2018

What was wrong?

As part of #1209 I ended up re-using some of the things from the fixture builder tests, and ended up doing a reasonably big re-organization of that code.

How was it fixed?

Re-organized the code from the old test_builder. This is mostly to get a bunch of unrelated changes out of #1209 before I wrap it up.

Cute Animal Picture

monkey_riding_a_dog

Copy link
Contributor

@carver carver left a comment

Choose a reason for hiding this comment

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

I really only looked at the overall directory structure, didn't look through all 3k line changes :P

New structure seems fine to me. 👍

@@ -34,10 +38,12 @@
from eth.rlp.headers import (
BlockHeader,
)
from eth.tools.fixture_tests import (
from eth.tools._utils.hashing import (
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh, I like this as an indication that the utils are meant to be for internal use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I want to push that pattern everywhere now that I've seen it in code.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @njgheorghita @cburgdorf @davesque @dylanjw for something that is fair game to push out across any/all of our repositories.

Copy link
Contributor

@carver carver Aug 29, 2018

Choose a reason for hiding this comment

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

I also added it to the default .gitignore for generated docs in the template: https://github.com/carver/ethereum-python-project-template/blob/master/.gitignore#L48

@pipermerriam pipermerriam merged commit 1a3d64f into ethereum:master Aug 28, 2018
@pipermerriam pipermerriam deleted the piper/re-organize-fixture-tools branch August 28, 2018 21:25
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.

None yet

2 participants