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

Use unique amcrest exceptions #108

Merged
merged 2 commits into from
Mar 11, 2019
Merged

Use unique amcrest exceptions #108

merged 2 commits into from
Mar 11, 2019

Conversation

pnbruckner
Copy link
Collaborator

Use amcrest specific exceptions where appropriate, such as for communication and login errors. Use retries and timeouts when generating authentation object. Check for invalid login when using Basic Authentication.

Use amcrest specific exceptions where appropriate, such as for communication and login errors. Use retries and timeouts when generating authentation object. Check for invalid login when using Basic Authentication.
src/amcrest/http.py Show resolved Hide resolved
src/amcrest/http.py Show resolved Hide resolved
src/amcrest/http.py Show resolved Hide resolved
src/amcrest/http.py Outdated Show resolved Hide resolved
src/amcrest/exceptions.py Show resolved Hide resolved
src/amcrest/exceptions.py Show resolved Hide resolved
src/amcrest/exceptions.py Show resolved Hide resolved
src/amcrest/__init__.py Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 33.333% when pulling 4458969 on exceptions into 31d616b on master.

@coveralls
Copy link

coveralls commented Mar 11, 2019

Coverage Status

Coverage increased (+0.3%) to 33.485% when pulling b5f2f4f on exceptions into 31d616b on master.

@pnbruckner
Copy link
Collaborator Author

One change I'm not absolutely sure about. I noticed in Http._generate_token that the response from the camera was only checked for "invalid" and "error" if Digest Authentication was used. Specifically, it did not do this test if Basic Authentication was used. I suspected this was an oversight, so I changed it to do the test no matter which authentication type was used. Is that correct? Or did it only do that check if Digest Authentication was used on purpose?

@@ -10,8 +10,8 @@
# GNU General Public License for more details.
#
# vim:sw=4:ts=4:et

from amcrest.http import Http
from .exceptions import AmcrestError, CommError, LoginError

Choose a reason for hiding this comment

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

'.exceptions.AmcrestError' imported but unused
'.exceptions.CommError' imported but unused
'.exceptions.LoginError' imported but unused

@pnbruckner
Copy link
Collaborator Author

Also, although these changes technically constitute an API change, it may not be worth going to version 2.0.0. 1.2.6 should be fine. Once these changes are released on pypi.org as a new version I'll update the HA amcrest components accordingly.

Copy link
Owner

@tchellomello tchellomello left a comment

Choose a reason for hiding this comment

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

Awesome @pnbruckner you rock!

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.

4 participants