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

Ethpm contract types property #1440

Merged

Conversation

njgheorghita
Copy link
Contributor

@njgheorghita njgheorghita commented Aug 29, 2019

What was wrong?

  • Added a Package.contract_types property to easily access all available contract types in a package.
  • Consolidated Package.from_uri tests

Todo:

Cute Animal Picture

image

@njgheorghita njgheorghita force-pushed the ethpm-contract-types-property branch 3 times, most recently from de818a4 to eac39c9 Compare August 29, 2019 16:26
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Looks good to me!

for deployment_name, deployment_data in deployments.items():
if deployment_data['contract_type'] not in contract_types:
if deployment_data['contract_type'] not in self.contract_types:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will you always have deployment data here? If not, it's probably worth a check:

if deployment_data and deployment_data['contract_type'] not in self.contract_types:
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh good thought - but we check for deployments on line 346 / so if people are using this private method correctly (via Package.deployments and not directly ) then we should be ok

@njgheorghita njgheorghita merged commit abb0e41 into ethereum:master Aug 29, 2019
@njgheorghita njgheorghita deleted the ethpm-contract-types-property branch August 29, 2019 16:57
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