-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: contract flattener #107
Conversation
Depends on release after ApeWorX/ape#1963 |
Co-authored-by: antazoey <jules@apeworx.io>
Co-authored-by: antazoey <jules@apeworx.io>
Co-authored-by: antazoey <jules@apeworx.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the biggest thing I would prefer is to get rid of the OUTFILE requirement.
Writing "out" should be done using the >>
operator.
ape vyper flatten path.vy >> out.vy
ape_vyper/compiler.py
Outdated
return None | ||
|
||
if version_spec is None: | ||
if version := first_full_release(self.available_versions): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we try to find an already matching installed version (self.installed_versions
) before checking the available versions?
I should be able to compile offline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's like 3 places that do this logic slightly differently. Would probably be good to refactor this a bit at some point. Not sure I have enough context to do that now so I'm leaving it alone.
How about 4af831e?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just double checked, it seems I am still able to compile without internet if I have the compiler installed, so that is good.
However, just to keep in mind:
1.) Turn off wifi.
2.) Launch ape console
.
3.) Run: compilers.vyper.available_versions
.
Notice it fails with:
ConnectionError: HTTPSConnectionPool(host='api.github.com', port=443): Max retries exceeded with url: /repos/vyperlang/vyper/releases?per_page=100 (Caused by NameResolutionError("<urllib3.connection.HTTPSConnection object at 0x131ed3790>: Failed to resolve 'api.github.com' ([Errno 8] nodename nor servname provided, or not known)"))
If the line self.available_versions
is executed without internet and catching the exception, the program will crash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's okay in this case. If there's no useful installed versions and we can't get available versions, then there's no path forward and we should bail, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct.
@cli.command(short_help="Flatten select contract source files") | ||
@ape_cli_context() | ||
@click.argument("CONTRACT", type=click.Path(exists=True, resolve_path=True)) | ||
@click.argument("OUTFILE", type=click.Path(exists=False, resolve_path=True, writable=True)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from testing: I think OUTFILE
is very un-unix like, especially because it is required.
As a CLI user, I would expect this to work:
ape vyper flatten FILE.vy >> outfile.vy
or also, having it output to stdout is kinda nice too:
ape vyper flatten File.vy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can achieve this by changing the impl to:
content = ape.compilers.vyper.flatten_contract(
Path(contract), base_path=ape.project.contracts_folder
)
click.echo(str(content))
This is how I originally wanted to do it but the ape logger ruins stdout. The click command can't get in and adjust log level before ape starts outputting stuff we don't want. Open to ideas. |
Co-authored-by: antazoey <jules@apeworx.io>
Co-authored-by: antazoey <jules@apeworx.io>
Co-authored-by: antazoey <jules@apeworx.io>
Co-authored-by: antazoey <jules@apeworx.io>
Co-authored-by: antazoey <jules@apeworx.io>
Co-authored-by: antazoey <jules@apeworx.io>
Co-authored-by: antazoey <jules@apeworx.io>
Co-authored-by: antazoey <jules@apeworx.io>
…ing during no-pragma fallback
one idea, let it default to stdout still so it isn't required. question, what are some of the earliest logs you were seeing? |
Maybe, but defaulting to stdout seems like it'll be very annoying to users when the output isn't a compilable contract. I'm not sure adjusting that decorator will do much when most of the output I've been seeing has come during plugin/config loading .
Couple examples. In a dev env:
In a clean project env:
Think I've seen some others as well but this is what I was able to get on first tries. We could suss out every warning or log output I can generate and suppress it but it doesn't really fix the fact that this is a common behavior of the ape logger. Unless we can somehow completely shut down logging at logger init time, I'm not sure we can reliably prevent stdout from being ruined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think we should not offer the CLI. Users could easily use Python to achieve the same thing, and it makes more sense because of the stdout thing.
What I did
Adds Vyper contract flattener functionality.
fixes: #75
How I did it
How to verify it
Checklist