search

terryyin / translate-python

Online translation as a Python module & command line tool. No key, no authentication needed.
MIT License
728 stars 153 forks source link

raise_for_status(), and Session for efficient http #92

Closed kxrob closed 2 years ago

terryyin commented 2 years ago

Thanks for the contribution. But where are the tests?

kxrob commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

thanks for the code and CI.

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