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

Provide an option to store error responses in CLI #47

Merged
merged 8 commits into from
Feb 1, 2024

Conversation

adnan-awan
Copy link
Contributor

@adnan-awan adnan-awan commented Sep 8, 2023

- Make initial change to write error response as well when --include-all-responses flag is passed.
- zytedata#38
zyte_api/__main__.py Outdated Show resolved Hide resolved
zyte_api/__main__.py Outdated Show resolved Hide resolved
@Gallaecio
Copy link
Contributor

In addition to those points, could you look into including a test for the change?

- Improvement by PR suggestion
- Add tests for the current changes.
- zytedata#38
- Refactor the test using mocks
- Add dependency
- zytedata#38
@Gallaecio Gallaecio mentioned this pull request Jan 17, 2024
1 task
- Remove RequestError import
- zytedata#38
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2024

Codecov Report

Merging #47 (69fd405) into main (5d561c4) will increase coverage by 15.25%.
Report is 27 commits behind head on main.
The diff coverage is 47.72%.

❗ Current head 69fd405 differs from pull request most recent head 068a001. Consider uploading reports for the commit 068a001 to get more accurate results

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

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #47       +/-   ##
===========================================
+ Coverage   38.63%   53.88%   +15.25%     
===========================================
  Files          10       10               
  Lines         365      373        +8     
  Branches       53       55        +2     
===========================================
+ Hits          141      201       +60     
+ Misses        223      167       -56     
- Partials        1        5        +4     
Files Coverage Δ
zyte_api/__version__.py 100.00% <100.00%> (ø)
zyte_api/constants.py 100.00% <100.00%> (ø)
zyte_api/utils.py 92.00% <100.00%> (+3.53%) ⬆️
zyte_api/aio/client.py 34.66% <66.66%> (+7.63%) ⬆️
zyte_api/aio/errors.py 56.25% <0.00%> (-12.99%) ⬇️
zyte_api/__main__.py 50.58% <45.71%> (+50.58%) ⬆️

... and 2 files with indirect coverage changes

tests/test_main.py Outdated Show resolved Hide resolved
tests/test_main.py Outdated Show resolved Hide resolved
tests/test_main.py Outdated Show resolved Hide resolved
tests/test_main.py Outdated Show resolved Hide resolved
zyte_api/__main__.py Outdated Show resolved Hide resolved
zyte_api/__main__.py Outdated Show resolved Hide resolved
- address the comments in PR
- zytedata#38
tests/test_main.py Outdated Show resolved Hide resolved
Comment on lines +42 to +46
def write_output(content):
json.dump(content, out, ensure_ascii=False)
out.write("\n")
out.flush()
pbar.update()
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL An inner function can depend on an outer scope variable as long as it is define before the inner function is called, rather than before the inner function is defined.

zyte_api/__main__.py Outdated Show resolved Hide resolved
zyte_api/__main__.py Outdated Show resolved Hide resolved
- address the comments in PR
- zytedata#38
- change the domain to forbidden.example
- zytedata#38
zyte_api/__main__.py Outdated Show resolved Hide resolved
@Gallaecio Gallaecio requested review from kmike, wRAR and BurnzZ January 24, 2024 14:12
- change the domain to forbidden.example
- zytedata#38
@Gallaecio Gallaecio merged commit 931a810 into zytedata:main Feb 1, 2024
6 of 7 checks passed
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.

Provide an option to store error responses in CLI
4 participants