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[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.
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.

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