Fix Original Envoy detection

jesserizzo / envoy_reader

MIT License

37 stars 26 forks source link

Fix Original Envoy detection #3

Closed DavidDeSloovere closed 5 years ago

DavidDeSloovere commented 5 years ago

The 200 check was ok, but the check on response.json() threw and exception. When requestion production.json, the original envoy redirects to /error (and still returning 200, bad practice).

Should be tested for Envoy S just to be sure.

DavidDeSloovere commented 5 years ago

I'm actualy improving this even more now.

DavidDeSloovere commented 5 years ago

@jesserizzo My first PR from a few weeks back add support for the original envoy (Envoy-C). But after some refactoring that got broken.

This PR fixes the detection.

Detection is also a lot more efficient now. Only on the first call will the model be detected and saved in the "model" variable. It could still use some improvements:

like one perform a single call to get all the data, instead of one call per field (or cache the response).
DONE check model in the consumption fields, and just return Unavailable if Envoy-C instead of making the call
read the correct json field depending on model instead of relying on try/except

(You should know I don't know any python except for what I wrote/learned for this envoy_reader, but I have been writing code for 20 years now.)

DavidDeSloovere commented 5 years ago

Don't know if there is still a need for and len(response.json()) == 3

jesserizzo commented 5 years ago

@DavidDeSloovere Instead of Model S and C maybe characterize them by feature set "supports_consumption" and "production_only" or something. The reason for this is there are some Envoy S that don't have the hardware for measuring consumption installed. This code should work for them, the len(response.json()) == 3 would detect them as a C.

Also, you're going back and forth between self.host and self.url

Other than that, looks great. Thanks.

DavidDeSloovere commented 5 years ago

@jesserizzo It's a bit more than just supporting consumption or not I think. Because json properties are also different. So I refactored towards that, eliminating the try/except flow. Don't know if it's the same in python, but exceptions are expensive in .NET and shouldn't be used for control flow.

Using self.host in message and self.url in requests. Also refactored those.

jesserizzo commented 5 years ago

@DavidDeSloovere Looks great, thanks!

DavidDeSloovere commented 5 years ago

Out of curiosity, how does this get on PyPi now?

jesserizzo commented 5 years ago

I just upload it manually. Which I just did. Thanks again.

DavidDeSloovere commented 5 years ago

Do you mind if I do the PR for the home assistant component? Haven't done one in that project yet and would like to try.

jesserizzo commented 5 years ago

Absolutely, go right ahead.