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

Introduce logging for failed requests. #28

Merged
merged 2 commits into from
Nov 8, 2016
Merged

Introduce logging for failed requests. #28

merged 2 commits into from
Nov 8, 2016

Conversation

sdague
Copy link
Contributor

@sdague sdague commented Nov 7, 2016

In response to Home Assistant bug -
home-assistant/core#4226

When things go wrong with a request in unexpected ways, we really
don't have enough information to figure out what just happened. This
is especially true with hardware generations that none of the upstream
developers have.

This adds logging support to the library, and introduces a couple of
error cases where we will log low level details of a failure to help
debug these kinds of issues in the future.

@wuub
Copy link
Owner

wuub commented Nov 7, 2016

Good idea. Can we improve it a little bit before merging? No real changes just nitpicking and readability :)

  1. In rxv/__init__.py lets add a NullHandler like this https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library
  2. Lowercase logger seems to be more common than uppercase LOG
  3. I think single rxv logger is enough for the whole library. So logger = logging.getLogger('rxv') instead of LOG = logging.getLogger(__name__)

@sdague
Copy link
Contributor Author

sdague commented Nov 7, 2016

All sounds like good feedback, I'll respin the patch with those improvements tonight.

In response to Home Assistant bug -
home-assistant/core#4226

When things go wrong with a request in unexpected ways, we really
don't have enough information to figure out what just happened. This
is especially true with hardware generations that none of the upstream
developers have.

This adds logging support to the library, and introduces a couple of
error cases where we will log low level details of a failure to help
debug these kinds of issues in the future.
play_status can get called on sources that don't support it, which
causes a vague XML exception to bubble up through the system. Much
better to be cautious and only call it on inputs we know support it.
@sdague
Copy link
Contributor Author

sdague commented Nov 8, 2016

Updated, plus a fix on top for Home Assistant bug - home-assistant/core#4226

@wuub wuub merged commit 9bb0ddb into wuub:master Nov 8, 2016
@wuub
Copy link
Owner

wuub commented Nov 8, 2016

thanks :)

@sdague
Copy link
Contributor Author

sdague commented Nov 8, 2016

Great, thanks! When you get chance, can we get another release out so we can do a req bump for the Home Assistant bug?

@wuub
Copy link
Owner

wuub commented Nov 8, 2016

In 8 to 10 hours.

@wuub
Copy link
Owner

wuub commented Nov 8, 2016

v0.3.1 is released

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