Skip to content

Make typing checks more strict #14429

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.

Sign up for GitHub

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

Merged
andrey-git merged 13 commits into from
Jul 13, 2018
Merged

Make typing checks more strict #14429

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
41ef7e7
Make typing checks more strict
andrey-git May 13, 2018
0d472d7
Add util/ to typing checks
andrey-git May 13, 2018
0c3a591
Unbreak dt code
andrey-git May 14, 2018
14afc72
Address comments
andrey-git May 14, 2018
2ffe76b
Address more comments
andrey-git May 14, 2018
821fcce
Merge branch 'dev' of https://github.com/home-assistant/home-assistan…
andrey-git Jul 9, 2018
d55cec7
Fix merge
andrey-git Jul 9, 2018
d80f613
Add new hass.async_create_task and fix callers
andrey-git Jul 10, 2018
35b1c3f
Fix tests
andrey-git Jul 11, 2018
55a69fa
Change
andrey-git Jul 11, 2018
61c5886
Remove unused param
andrey-git Jul 11, 2018
0bd1259
Cleanup
andrey-git Jul 12, 2018
193d12f
Remove outdated test
andrey-git Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions homeassistant/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ def cmdline() -> List[str]:


def setup_and_run_hass(config_dir: str,
args: argparse.Namespace) -> Optional[int]:
args: argparse.Namespace) -> int:
"""Set up HASS and run."""
from homeassistant import bootstrap

Expand Down Expand Up @@ -274,7 +274,7 @@ def setup_and_run_hass(config_dir: str,
log_no_color=args.log_no_color)

if hass is None:
return None
return -1

if args.open_ui:
# Imported here to avoid importing asyncio before monkey patch
Expand Down
10 changes: 6 additions & 4 deletions homeassistant/bootstrap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,8 @@ def from_config_file(config_path: str,
skip_pip: bool = True,
log_rotate_days: Any = None,
log_file: Any = None,
log_no_color: bool = False):
log_no_color: bool = False)\
-> Optional[core.HomeAssistant]:
"""Read the configuration file and try to start all the functionality.

Will add functionality to 'hass' parameter if given,
Expand All @@ -188,7 +189,8 @@ async def async_from_config_file(config_path: str,
skip_pip: bool = True,
log_rotate_days: Any = None,
log_file: Any = None,
log_no_color: bool = False):
log_no_color: bool = False)\
-> Optional[core.HomeAssistant]:
"""Read the configuration file and try to start all the functionality.

Will add functionality to 'hass' parameter.
Expand All @@ -211,9 +213,9 @@ async def async_from_config_file(config_path: str,
finally:
clear_secret_cache()

hass = await async_from_config_dict(
updated_hass = await async_from_config_dict(
config_dict, hass, enable_log=False, skip_pip=skip_pip)
return hass
return updated_hass


@core.callback
Expand Down
7 changes: 5 additions & 2 deletions homeassistant/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ def add_job(self, target: Callable[..., None], *args: Any) -> None:
def async_add_job(
self,
target: Callable[..., Any],
*args: Any) -> Optional[asyncio.tasks.Task]:
*args: Any) -> asyncio.tasks.Task:
"""Add a job from within the eventloop.

This method must be run in the event loop.
Expand All @@ -218,7 +218,10 @@ def async_add_job(
if asyncio.iscoroutine(target):
task = self.loop.create_task(target)
elif is_callback(target):
self.loop.call_soon(target, *args)
async def wrapper() -> Any:
"""Wrapper coroutine around a callback."""
return target(*args)
task = self.loop.create_task(wrapper())

@pvizeli pvizeli May 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't wrap it into a slow coroutine.

@andrey-git andrey-git May 14, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions how to properly do that?

  1. If the caller knows that the target is a @callback and is not afraid of exceptions - they can call it directly.

  2. If the caller knows that the target is a @callback, they are afraid of exceptions and don't care about return value - they can call loop.call_soon manually (i.e. the previous implementation)

  3. If the caller knows that the target is a @callback, they are afraid of exceptions and they want the return value - the old implementation didn't provide that, the new one does.

  4. If the caller doesn't know what target is and they need the return value - same as (3)

  5. If the caller doesn't know what target is and they don't need the return value - I agree that the old implementation is better, but it would be also better for coro not to be wrapped in task for such a case.

Generally having a single function with different return types is bad.

@pvizeli pvizeli May 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Generally having a single function with different return types is bad.

For typing, yes but not for python itself

This change is very bad for the core and extend the memory footprint. We should hold the core slim and don't blow it up for a not exists type checking. Python is not strict and I hope that will be change in 4.x like c++ but actually I see no reason to make this bad change. In that case, I dislike this.

they can call loop.call_soon manually

And no, user should use our internal job handler or events stuff and not use the raw asyncio functions.

And the real solution is to remove @callback and make all that coreroutine. But it is good like it is now.

@balloob balloob May 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So I would say that it's kinda a miracle that not returning anything has never caused any issues. The majority of the callbacks in use are actually being used inside helpers/event.py. There we call hass.async_run_job which will run callbacks right away and enqueue the other types of functions.

Let's measure how much overhead this adds with our benchmark module.

hass --script benchmark async_million_state_changed_helper

@balloob balloob May 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On dev

› hass --script benchmark async_million_state_changed_helper
Using event loop: asyncio.unix_events
Benchmark async_million_state_changed_helper done in 4.237771602987777s
Benchmark async_million_state_changed_helper done in 4.250693870999385s
Benchmark async_million_state_changed_helper done in 4.256614304991672s
Benchmark async_million_state_changed_helper done in 4.24649005298852s

With this PR:

› hass --script benchmark async_million_state_changed_helper
Using event loop: asyncio.unix_events
Benchmark async_million_state_changed_helper done in 5.726596449007047s
Benchmark async_million_state_changed_helper done in 5.9516269309970085s
Benchmark async_million_state_changed_helper done in 5.672646453007474s
Benchmark async_million_state_changed_helper done in 5.7938863370072795s

That's just a quick check of running it a couple of times. That's a significant slowdown.

@balloob balloob May 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(this converted both the listener inside the benchmark as the function defined inside async_track_state_change to async functions)

@balloob balloob May 30, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interestingly, if I just convert the function inside track_state_change to be async, I get back to ~5.5 seconds per run

@andrey-git andrey-git May 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think a proper solution is to make 5 new (type-safe) functions for all the usecases I mentioned above, then convert the callers and then delete the functions that are not actually used

@balloob balloob Jun 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with that approach.

@pvizeli pvizeli Jun 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm. That is python and not type safe. To make the code complex for nothing is not realy cool. If that is a c++ project, I would agree but not with python...

elif asyncio.iscoroutinefunction(target):
task = self.loop.create_task(target(*args))
else:
Expand Down
9 changes: 5 additions & 4 deletions homeassistant/loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
from types import ModuleType

# pylint: disable=unused-import
from typing import Dict, List, Optional, Sequence, Set # NOQA
from typing import Dict, List, Optional, Sequence, Set, TYPE_CHECKING # NOQA

from homeassistant.const import PLATFORM_FORMAT
from homeassistant.util import OrderedSet

# Typing imports
# Typing imports that create a circular dependency
# pylint: disable=using-constant-test,unused-import
if False:
if TYPE_CHECKING:
from homeassistant.core import HomeAssistant # NOQA

PREPARED = False
Expand All @@ -39,7 +39,8 @@
PACKAGE_COMPONENTS = 'homeassistant.components'


def set_component(hass, comp_name: str, component: ModuleType) -> None:
def set_component(hass, # type: HomeAssistant
comp_name: str, component: Optional[ModuleType]) -> None:
"""Set a component in the cache.

Async friendly.
Expand Down
17 changes: 4 additions & 13 deletions homeassistant/util/color.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ def color_name_to_rgb(color_name):
return hex_value


# pylint: disable=invalid-name, invalid-sequence-index
# pylint: disable=invalid-name
def color_RGB_to_xy(iR: int, iG: int, iB: int) -> Tuple[float, float]:
"""Convert from RGB color to XY color."""
return color_RGB_to_xy_brightness(iR, iG, iB)[:2]
Expand All @@ -182,7 +182,7 @@ def color_RGB_to_xy(iR: int, iG: int, iB: int) -> Tuple[float, float]:
# Taken from:
# http://www.developers.meethue.com/documentation/color-conversions-rgb-xy
# License: Code is given as is. Use at your own risk and discretion.
# pylint: disable=invalid-name, invalid-sequence-index
# pylint: disable=invalid-name
def color_RGB_to_xy_brightness(
iR: int, iG: int, iB: int) -> Tuple[float, float, int]:
"""Convert from RGB color to XY color."""
Expand Down Expand Up @@ -224,7 +224,6 @@ def color_xy_to_RGB(vX: float, vY: float) -> Tuple[int, int, int]:

# Converted to Python from Obj-C, original source from:
# http://www.developers.meethue.com/documentation/color-conversions-rgb-xy
# pylint: disable=invalid-sequence-index
def color_xy_brightness_to_RGB(vX: float, vY: float,
ibrightness: int) -> Tuple[int, int, int]:
"""Convert from XYZ to RGB."""
Expand Down Expand Up @@ -265,12 +264,11 @@ def color_xy_brightness_to_RGB(vX: float, vY: float,
return (ir, ig, ib)


# pylint: disable=invalid-sequence-index
def color_hsb_to_RGB(fH: float, fS: float, fB: float) -> Tuple[int, int, int]:
"""Convert a hsb into its rgb representation."""
if fS == 0:
fV = fB * 255
return (fV, fV, fV)
fV = int(fB * 255)
return fV, fV, fV

r = g = b = 0
h = fH / 60
Expand Down Expand Up @@ -307,7 +305,6 @@ def color_hsb_to_RGB(fH: float, fS: float, fB: float) -> Tuple[int, int, int]:
return (r, g, b)


# pylint: disable=invalid-sequence-index
def color_RGB_to_hsv(iR: int, iG: int, iB: int) -> Tuple[float, float, float]:
"""Convert an rgb color to its hsv representation.

Expand All @@ -319,13 +316,11 @@ def color_RGB_to_hsv(iR: int, iG: int, iB: int) -> Tuple[float, float, float]:
return round(fHSV[0]*360, 3), round(fHSV[1]*100, 3), round(fHSV[2]*100, 3)


# pylint: disable=invalid-sequence-index
def color_RGB_to_hs(iR: int, iG: int, iB: int) -> Tuple[float, float]:
"""Convert an rgb color to its hs representation."""
return color_RGB_to_hsv(iR, iG, iB)[:2]


# pylint: disable=invalid-sequence-index
def color_hsv_to_RGB(iH: float, iS: float, iV: float) -> Tuple[int, int, int]:
"""Convert an hsv color into its rgb representation.

Expand All @@ -337,26 +332,22 @@ def color_hsv_to_RGB(iH: float, iS: float, iV: float) -> Tuple[int, int, int]:
return (int(fRGB[0]*255), int(fRGB[1]*255), int(fRGB[2]*255))


# pylint: disable=invalid-sequence-index
def color_hs_to_RGB(iH: float, iS: float) -> Tuple[int, int, int]:
"""Convert an hsv color into its rgb representation."""
return color_hsv_to_RGB(iH, iS, 100)


# pylint: disable=invalid-sequence-index
def color_xy_to_hs(vX: float, vY: float) -> Tuple[float, float]:
"""Convert an xy color to its hs representation."""
h, s, _ = color_RGB_to_hsv(*color_xy_to_RGB(vX, vY))
return (h, s)


# pylint: disable=invalid-sequence-index
def color_hs_to_xy(iH: float, iS: float) -> Tuple[float, float]:
"""Convert an hs color to its xy representation."""
return color_RGB_to_xy(*color_hs_to_RGB(iH, iS))


# pylint: disable=invalid-sequence-index
def _match_max_scale(input_colors: Tuple[int, ...],
output_colors: Tuple[int, ...]) -> Tuple[int, ...]:
"""Match the maximum value of the output to the input."""
Expand Down
26 changes: 13 additions & 13 deletions homeassistant/util/dt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
from typing import Any, Dict, Union, Optional, Tuple # NOQA

import pytz
import pytz.exceptions as pytzexceptions

DATE_STR_FORMAT = "%Y-%m-%d"
UTC = DEFAULT_TIME_ZONE = pytz.utc # type: dt.tzinfo
UTC = pytz.utc
DEFAULT_TIME_ZONE = pytz.utc # type: dt.tzinfo


# Copyright (c) Django Software Foundation and individual contributors.
Expand Down Expand Up @@ -42,7 +44,7 @@ def get_time_zone(time_zone_str: str) -> Optional[dt.tzinfo]:
"""
try:
return pytz.timezone(time_zone_str)
except pytz.exceptions.UnknownTimeZoneError:
except pytzexceptions.UnknownTimeZoneError:
return None


Expand All @@ -64,7 +66,7 @@ def as_utc(dattim: dt.datetime) -> dt.datetime:
if dattim.tzinfo == UTC:
return dattim
elif dattim.tzinfo is None:
dattim = DEFAULT_TIME_ZONE.localize(dattim)
dattim = DEFAULT_TIME_ZONE.localize(dattim) # type: ignore

return dattim.astimezone(UTC)

Expand Down Expand Up @@ -92,7 +94,7 @@ def as_local(dattim: dt.datetime) -> dt.datetime:

def utc_from_timestamp(timestamp: float) -> dt.datetime:
"""Return a UTC time from a timestamp."""
return dt.datetime.utcfromtimestamp(timestamp).replace(tzinfo=UTC)
return UTC.localize(dt.datetime.utcfromtimestamp(timestamp))

@balloob balloob Jul 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this the same? Is a non timezone aware date that is being localized treated as having been UTC time? Or would it treat it as local time?

@andrey-git andrey-git Jul 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here is the implementation of UTC.localize

    def localize(self, dt, is_dst=False):
        '''Convert naive time to local time'''
        if dt.tzinfo is not None:
            raise ValueError('Not naive datetime (tzinfo is already set)')
        return dt.replace(tzinfo=self)



def start_of_local_day(dt_or_d:
Expand All @@ -102,13 +104,14 @@ def start_of_local_day(dt_or_d:
date = now().date() # type: dt.date
elif isinstance(dt_or_d, dt.datetime):
date = dt_or_d.date()
return DEFAULT_TIME_ZONE.localize(dt.datetime.combine(date, dt.time()))
return DEFAULT_TIME_ZONE.localize(dt.datetime.combine( # type: ignore
date, dt.time()))


# Copyright (c) Django Software Foundation and individual contributors.
# All rights reserved.
# https://github.com/django/django/blob/master/LICENSE
def parse_datetime(dt_str: str) -> dt.datetime:
def parse_datetime(dt_str: str) -> Optional[dt.datetime]:
"""Parse a string and return a datetime.datetime.

This function supports time zone offsets. When the input contains one,
Expand All @@ -134,14 +137,12 @@ def parse_datetime(dt_str: str) -> dt.datetime:
if tzinfo_str[0] == '-':
offset = -offset
tzinfo = dt.timezone(offset)
else:
tzinfo = None
kws = {k: int(v) for k, v in kws.items() if v is not None}
kws['tzinfo'] = tzinfo
return dt.datetime(**kws)


def parse_date(dt_str: str) -> dt.date:
def parse_date(dt_str: str) -> Optional[dt.date]:
"""Convert a date string to a date object."""
try:
return dt.datetime.strptime(dt_str, DATE_STR_FORMAT).date()
Expand Down Expand Up @@ -180,9 +181,8 @@ def get_age(date: dt.datetime) -> str:
def formatn(number: int, unit: str) -> str:
"""Add "unit" if it's plural."""
if number == 1:
return "1 %s" % unit
elif number > 1:
return "%d %ss" % (number, unit)
return '1 {}'.format(unit)
return '{:d} {}s'.format(number, unit)

# pylint: disable=invalid-sequence-index
def q_n_r(first: int, second: int) -> Tuple[int, int]:
Expand Down Expand Up @@ -211,4 +211,4 @@ def q_n_r(first: int, second: int) -> Tuple[int, int]:
if minute > 0:
return formatn(minute, 'minute')

return formatn(second, 'second') if second > 0 else "0 seconds"
return formatn(second, 'second')
11 changes: 4 additions & 7 deletions homeassistant/util/json.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@

_LOGGER = logging.getLogger(__name__)

_UNDEFINED = object()


def load_json(filename: str, default: Union[List, Dict] = _UNDEFINED) \
def load_json(filename: str, default: Union[List, Dict, None] = None) \
-> Union[List, Dict]:
"""Load JSON data from a file and return as dict or list.

Expand All @@ -29,7 +27,7 @@ def load_json(filename: str, default: Union[List, Dict] = _UNDEFINED) \
except OSError as error:
_LOGGER.exception('JSON file reading failed: %s', filename)
raise HomeAssistantError(error)
return {} if default is _UNDEFINED else default
return {} if default is None else default

@balloob balloob Jul 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This not the same. By removing the sentinel value, it is no longer possible to have None be the default value.

@andrey-git andrey-git Jul 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There were no uses of None as default value except the place I changed in storage.py

@balloob balloob Jul 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There was no use yet, as the storage helper is very new. I don't think that we should change this.

@andrey-git andrey-git Jul 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The storage helper was the lone use. I meant that no one else used None for load_json

By using sentinel value the function becomes not type-safe, as it accepts object and thus could return object if one were to pass it as default value.

Another option is to have {} as the default value and ignore that warning that generates with a comment that we are not changing it, nor do we return it as-is for the caller to modify.

@balloob balloob Jul 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't feel too strongly I guess.

Any place that was using util.json or util.yaml needs to be rewritten to use the storage helper anyway.

@balloob balloob Jul 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, you could also make the sentinal value an empty dict, that way you can keep it all as dict. Comparison is still done with is so will work the same.

@andrey-git andrey-git Jul 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It will generate a warning that the default value is mutable.

Lets keep it None



def save_json(filename: str, data: Union[List, Dict]):
Expand All @@ -38,9 +36,9 @@ def save_json(filename: str, data: Union[List, Dict]):
Returns True on success.
"""
try:
data = json.dumps(data, sort_keys=True, indent=4)
json_data = json.dumps(data, sort_keys=True, indent=4)
with open(filename, 'w', encoding='utf-8') as fdesc:
fdesc.write(data)
fdesc.write(json_data)
return True
except TypeError as error:
_LOGGER.exception('Failed to serialize to JSON: %s',
Expand All @@ -50,4 +48,3 @@ def save_json(filename: str, data: Union[List, Dict]):
_LOGGER.exception('Saving JSON file failed: %s',
filename)
raise HomeAssistantError(error)
return False
6 changes: 3 additions & 3 deletions homeassistant/util/unit_system.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,11 @@ def __init__(self: object, name: str, temperature: str, length: str,
self.volume_unit = volume

@property
def is_metric(self: object) -> bool:
def is_metric(self) -> bool:
"""Determine if this is the metric unit system."""
return self.name == CONF_UNIT_SYSTEM_METRIC

def temperature(self: object, temperature: float, from_unit: str) -> float:
def temperature(self, temperature: float, from_unit: str) -> float:
"""Convert the given temperature to this unit system."""
if not isinstance(temperature, Number):
raise TypeError(
Expand All @@ -99,7 +99,7 @@ def temperature(self: object, temperature: float, from_unit: str) -> float:
return temperature_util.convert(temperature,
from_unit, self.temperature_unit)

def length(self: object, length: float, from_unit: str) -> float:
def length(self, length: float, from_unit: str) -> float:
"""Convert the given length to this unit system."""
if not isinstance(length, Number):
raise TypeError('{} is not a numeric value.'.format(str(length)))
Expand Down
6 changes: 4 additions & 2 deletions homeassistant/util/yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def compose_node(self, parent: yaml.nodes.Node, index) -> yaml.nodes.Node:
last_line = self.line # type: int
node = super(SafeLineLoader,
self).compose_node(parent, index) # type: yaml.nodes.Node
node.__line__ = last_line + 1
node.__line__ = last_line + 1 # type: ignore
return node


Expand All @@ -69,7 +69,7 @@ def load_yaml(fname: str) -> Union[List, Dict]:
# We convert that to an empty dict
return yaml.load(conf_file, Loader=SafeLineLoader) or OrderedDict()
except yaml.YAMLError as exc:
_LOGGER.error(exc)
_LOGGER.error(str(exc))

@MartinHjelmare MartinHjelmare May 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Exceptions have a __str__ magic method so this shouldn't be needed.

@andrey-git andrey-git May 14, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

mypy complains that it is expecting str and got an Exception. Rather than add # type: ignore I think an explicit cast is more readable.

@MartinHjelmare MartinHjelmare May 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think mypy is wrong in this case. I'd prefer the ignore, but you decide, 🤷‍♂️.

@balloob balloob May 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I am leaning towards str

raise HomeAssistantError(exc)
except UnicodeDecodeError as exc:
_LOGGER.error("Unable to read file %s: %s", fname, exc)
Expand Down Expand Up @@ -232,6 +232,8 @@ def _load_secret_yaml(secret_path: str) -> Dict:
_LOGGER.debug('Loading %s', secret_path)
try:
secrets = load_yaml(secret_path)
if not isinstance(secrets, dict):
raise HomeAssistantError('Secrets is not a dictionary')
if 'logger' in secrets:
logger = str(secrets['logger']).lower()
if logger == 'debug':
Expand Down
6 changes: 4 additions & 2 deletions tests/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ def test_async_add_job_schedule_callback():
job = MagicMock()

ha.HomeAssistant.async_add_job(hass, ha.callback(job))
assert len(hass.loop.call_soon.mock_calls) == 1
assert len(hass.loop.create_task.mock_calls) == 0
assert len(hass.loop.call_soon.mock_calls) == 0
assert len(hass.loop.create_task.mock_calls) == 1
assert len(hass.add_job.mock_calls) == 0


Expand Down Expand Up @@ -216,6 +216,8 @@ def wait_finish_callback():
run_coroutine_threadsafe(
wait_finish_callback(), self.hass.loop).result()

assert len(self.hass._pending_tasks) == 2

self.hass.block_till_done()

assert len(self.hass._pending_tasks) == 0
Expand Down
Loading