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

fix: make .extract_manifest() also compile if needed #2248

Merged
merged 7 commits into from
Aug 27, 2024

Conversation

antazoey
Copy link
Member

What I did

pre 0.8. this happened, and I don't see why it should not happen, so making it happen again.
(here you go @Ninjagod1251 )

How I did it

How to verify it

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

@antazoey
Copy link
Member Author

Some manual testing showing it working as well:

In [7]: project.manifest.contract_types = {}

In [8]: mani_pedi = project.extract_manifest()
INFO (ape-vyper): Compiling using Vyper compiler '0.4.0'.
Input:
        contracts/Token.vy

In [9]: mani_pedi = project.extract_manifest()

In [10]: mani_pedi = project.extract_manifest()

@antazoey
Copy link
Member Author

antazoey commented Aug 27, 2024

  • Make also happen in manifest projects

fubuloubu
fubuloubu previously approved these changes Aug 27, 2024
try:
self.load_contracts()
except CompilerError as err:
logger.debug(err)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be higher? Maybe warning

Copy link
Member Author

Choose a reason for hiding this comment

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

perhaps! I am just worried about dependencies trying to extract manifests from that aren't meant to be compiled.

Copy link
Member Author

Choose a reason for hiding this comment

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

i suppose they could just .manifest in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to warning but added a comment. let me know if that makes sense, along with my concerns.

Copy link
Member

Choose a reason for hiding this comment

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

perhaps! I am just worried about dependencies trying to extract manifests from that aren't meant to be compiled.

Wonder if it makes more sense as like .enrich_manifest or .fully_compiled_manifest, or something

@@ -2461,7 +2461,7 @@ def extract_manifest(self) -> PackageManifest:
sources = dict(self.sources)
contract_types = {
n: ct
for n, ct in (self.manifest.contract_types or {}).items()
for n, ct in self.load_contracts().items()
Copy link
Contributor

Choose a reason for hiding this comment

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

so now load_contracts and extract manifest outputs the results i need?

Copy link
Member Author

Choose a reason for hiding this comment

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

you can delete the .load_contracts() line, yes (once 0.8.14 is out).
However, it also doesn't hurt to have it.

@antazoey antazoey enabled auto-merge (squash) August 27, 2024 18:25
@antazoey antazoey merged commit 9e932b8 into ApeWorX:main Aug 27, 2024
16 checks passed
@antazoey antazoey deleted the fix/compile-on-extract branch August 27, 2024 18:56
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.

3 participants