Skip to content

raise_for_status(), and Session for efficient http#92

Merged
terryyin merged 8 commits intoterryyin:masterfrom
kxrob:raise_http_error
Mar 2, 2022
Merged

raise_for_status(), and Session for efficient http#92
terryyin merged 8 commits intoterryyin:masterfrom
kxrob:raise_http_error

Conversation

@kxrob
Copy link
Copy Markdown
Contributor

@kxrob kxrob commented Feb 24, 2022

No description provided.

@terryyin
Copy link
Copy Markdown
Owner

Thanks for the contribution. But where are the tests?

So that error messages do not go through silently
as translation results - e.g. when hitting usage
limits.
@kxrob
Copy link
Copy Markdown
Contributor Author

kxrob commented Feb 28, 2022

Thanks for the contribution. But where are the tests?

It doesn't really add new functionality to test. Positive function is already tested by the existing tests.
(Added a test provoking an HTTPError, but not sure if reasonable.)

@terryyin
Copy link
Copy Markdown
Owner

terryyin commented Mar 1, 2022

@kxrob the test looks good to me. Very interesting testing approach. I learned something. Thanks.

There's some duplicate code, do you want to remove them? Eg. extract them together, and/or, pull it up to the caller?

@kxrob kxrob force-pushed the raise_http_error branch from ee488d9 to 5138a3b Compare March 1, 2022 14:08
@kxrob
Copy link
Copy Markdown
Contributor Author

kxrob commented Mar 1, 2022

There's some duplicate code

Is this about the 2 lines for session setup in _make_request ?
Its not well equal and interleaved (and Libre has external library); only 2 lines equal on some providers. Pulling session init to __init__ (super class) would delay imports etc. before real action, and not every provider needs it. Usage of requests library could also change per provider.
So have no good idea so far how to compress it nice?

I added automatic github CI testing - upon pull request pushes and upon push to master branch (and to a _ci_test branch for experiments). The feature would require to be activated in your github repository - in the "Actions" tab of the repo. It ran in my fork (here: https://github.com/kxrob/translate-python/tree/_ci_test )

@terryyin terryyin merged commit 8e4993e into terryyin:master Mar 2, 2022
@terryyin
Copy link
Copy Markdown
Owner

terryyin commented Mar 2, 2022

thanks for the code and CI.

I didn't need to activated github actions. Just merging your pull request activated it.

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.

2 participants