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

Output flag #2452

Merged
merged 2 commits into from
Sep 30, 2021
Merged

Output flag #2452

merged 2 commits into from
Sep 30, 2021

Conversation

benjiqq
Copy link
Contributor

@benjiqq benjiqq commented Sep 6, 2021

What I did

add the flag "-o" to generate output files. this will write abi, bin and combined files. other options like ast, lll, opcodes need to be handled separately.

How I did it

add a flag to argparser and write the files

How to verify it

Description for the changelog

@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2021

Codecov Report

Merging #2452 (c686e8b) into master (366e133) will decrease coverage by 0.02%.
The diff coverage is 46.66%.

❗ Current head c686e8b differs from pull request most recent head cc4e024. Consider uploading reports for the commit cc4e024 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2452      +/-   ##
==========================================
- Coverage   84.66%   84.63%   -0.03%     
==========================================
  Files          94       94              
  Lines        9446     9453       +7     
  Branches     2208     2209       +1     
==========================================
+ Hits         7997     8001       +4     
- Misses        939      941       +2     
- Partials      510      511       +1     
Impacted Files Coverage Δ
vyper/cli/vyper_compile.py 74.16% <46.66%> (-1.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 366e133...cc4e024. Read the comment docs.

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

I think this PR will read a lot cleaner if you use a single branch, and just select a file handle depending on the option. If -o is set, set f to args.output_path. Otherwise, set it to sys.stdout.

vyper/cli/vyper_compile.py Outdated Show resolved Hide resolved
vyper/cli/vyper_compile.py Outdated Show resolved Hide resolved
@benjiqq
Copy link
Contributor Author

benjiqq commented Sep 8, 2021

I think this PR will read a lot cleaner if you use a single branch, and just select a file handle depending on the option. If -o is set, set f to args.output_path. Otherwise, set it to sys.stdout.

agreed, will do

@benjiqq
Copy link
Contributor Author

benjiqq commented Sep 10, 2021

@charles-cooper changed the PR as you suggested. if two arguments for the flags are passed doesn't work well.

vyper foo.vy -f bytecode,abi -o foo.txt writes 1 file without breakline

not really sure, in that case auto recognizing the format would be useful (write an abi and bin file separately).

vyper/cli/vyper_compile.py Outdated Show resolved Hide resolved
vyper/cli/vyper_compile.py Outdated Show resolved Hide resolved
@benjiqq benjiqq force-pushed the outputflag branch 2 times, most recently from 011aafd to a11b0a4 Compare September 23, 2021 10:00
@charles-cooper
Copy link
Member

In order to avoid managing the resource manually (i.e. inserting calls to f.close() before every escape path), i think a bit of refactoring is in order. To abstract which file handle is being used, you could use a helper function

def cli_helper(f):
  ...

and then you would do something like

if args.output_path:
  with open(...) as f:
    cli_helper(f)
else:
  cli_helper(f)

@charles-cooper
Copy link
Member

Also, please use print(..., file=f) instead of f.write(). The latter is a break with existing semantics -- it breaks existing flags (like -f ir)

@benjiqq
Copy link
Contributor Author

benjiqq commented Sep 29, 2021

@charles-cooper updated. I added end="" so that the output does not end with newline.

@charles-cooper
Copy link
Member

charles-cooper commented Sep 29, 2021

@charles-cooper updated. I added end="" so that the output does not end with newline.

I think we can accept this PR but without the end="". That breaks the existing output format. I am open to the format change, but we should have an explicit discussion about it - either going through the VIP process or at least asking the major consumers of vyper if that's a breaking change for them first.

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

Final nit: (style guide: functions private to a module should be prefixed with _)

vyper/cli/vyper_compile.py Outdated Show resolved Hide resolved
vyper/cli/vyper_compile.py Outdated Show resolved Hide resolved
vyper/cli/vyper_compile.py Outdated Show resolved Hide resolved
@charles-cooper charles-cooper merged commit 030bde6 into vyperlang:master Sep 30, 2021
skellet0r pushed a commit to skellet0r/vyper that referenced this pull request Oct 3, 2021
Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>
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.

4 participants