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

Write Compiler class #2

Merged
merged 2 commits into from
Oct 2, 2018
Merged

Conversation

njgheorghita
Copy link
Collaborator

What was wrong?

#1

How was it fixed?

For the most part, I followed what was outlined in #1, but added output (i.e. the compiler_output) as an attribute to Compiler, since it falls more in line with how py-ethpm's builder functions are currently designed.

An alternative would be to adjust the builder functions in py-ethpm to support something like

package = b.build(
    {},
    ...,  # initialize basic package meta
    inline_sources(sources),  # inlines all of the sources
    contract_type("Owned", contract_types),  #
)

But I thought this would be a better way to get this working initially

Cute Animal Picture

image

@njgheorghita
Copy link
Collaborator Author

@pipermerriam pinging for initial review - CI won't pass until pytest-ethereum is released which i'm working on now

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Generally 👍

In a subsequent PR, it might be good to start establishing the basic documentation for the API as well so that the way it is intended to be used can begin being defined.

tox.ini Outdated
@@ -39,4 +40,5 @@ basepython=python
extras=lint
commands=
flake8 {toxinidir}/twig {toxinidir}/tests
isort --recursive --check-only --diff {toxinidir}/twig {toxinidir}/tests
black --check --diff {toxinidir}/twig/ --check --diff {toxinidir}/tests/
Copy link
Member

Choose a reason for hiding this comment

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

tabs

def generate_inline_sources(compiler_output: Dict[str, Any]):
for path in compiler_output.keys():
contract_type = path.split("/")[-1].split(".")[0]
yield b.inline_source(contract_type, compiler_output)
Copy link
Member

Choose a reason for hiding this comment

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

neat that the builder tool can be used here.

SOURCES_GLOB = "**/*.vy"


def collect_sources(path, glob=SOURCES_GLOB):
Copy link
Member

Choose a reason for hiding this comment

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

In theory, this function would be a public API which would imply it should live outside of utils (which should be _utils under our new paradigm)

def __init__(self, sources, backend):
self.sources = sources
self.backend = backend
self.output = self.backend.compile(self.sources)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a computed property rather than populating it eagerly during initialization. No strong reason other than compilation often being expensive.

@njgheorghita njgheorghita force-pushed the init-dev branch 6 times, most recently from a7df574 to 71b9ed2 Compare October 2, 2018 17:38
@njgheorghita njgheorghita merged commit c7d3558 into ethereum:master Oct 2, 2018
@njgheorghita njgheorghita deleted the init-dev branch October 2, 2018 17:41
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.

2 participants