Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix[tool]: fix combined_json output for CLI #3901

Merged
merged 6 commits into from
Apr 2, 2024

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Apr 1, 2024

What I did

fix #3900

How I did it

How to verify it

Commit message

the output json would not be produced because Path does not have a json
serializer

there are actually tests for `combined_json`, but they test the
`compile_files` API directly, whereas the offending code is in the very
outer `_cli_helper()` function.

the best (long-term) way to test this might be to have a harness which
runs the vyper CLI directly from shell, but that is not explored here
to reduce scope.

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

the output json would not be produced because Path does not have a json
serializer
@charles-cooper charles-cooper marked this pull request as ready for review April 1, 2024 16:49
@charles-cooper charles-cooper changed the title fix[tool]: fix combined_json output for CLI fix[tool]: fix combined_json output for CLI Apr 1, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 59.26%. Comparing base (5bdd174) to head (2d8978c).
Report is 1 commits behind head on master.

❗ Current head 2d8978c differs from pull request most recent head 3011847. Consider uploading reports for the commit 3011847 to get more accurate results

Files Patch % Lines
vyper/cli/vyper_compile.py 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3901       +/-   ##
===========================================
- Coverage   86.33%   59.26%   -27.08%     
===========================================
  Files          92       92               
  Lines       14036    14036               
  Branches     3085     3085               
===========================================
- Hits        12118     8318     -3800     
- Misses       1490     5090     +3600     
- Partials      428      628      +200     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@charles-cooper charles-cooper enabled auto-merge (squash) April 2, 2024 17:50
@charles-cooper charles-cooper merged commit e34ca9c into vyperlang:master Apr 2, 2024
148 checks passed
electriclilies pushed a commit to electriclilies/vyper that referenced this pull request Apr 27, 2024
the output json would not be produced because Path does not have a json
serializer

there are actually tests for `combined_json`, but they test the
`compile_files` API directly, whereas the offending code is in the very
outer `_cli_helper()` function.

the best (long-term) way to test this might be to have a harness which
runs the vyper CLI directly from shell, but that is not explored here
to reduce scope.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keys must be str, int, float, bool or None, not PosixPath
3 participants