From 4be3cb2571ba6042ed9349e593016dbf0d2af178 Mon Sep 17 00:00:00 2001 From: Lucas Hoffmann Date: Mon, 23 Dec 2024 10:17:08 +0100 Subject: [PATCH] Refactor _get_multi_property into get_all; make name getters nullable The _list attribute on the underlying vcard object should be faster than iterating through all attributes ourself. With this it should be possible to detect if an attribute is present on the vcard or not. If it is not present the @property on the wrapper returns None or an empty list or an empty dict, depending on the attribute. --- khard/contacts.py | 118 +++++++++++++++++++------------------ test/test_vcard_wrapper.py | 15 ++++- 2 files changed, 75 insertions(+), 58 deletions(-) diff --git a/khard/contacts.py b/khard/contacts.py index b74b26a..c74819d 100644 --- a/khard/contacts.py +++ b/khard/contacts.py @@ -39,6 +39,7 @@ logger = logging.getLogger(__name__) T = TypeVar("T") +LabeledStrs = list[Union[str, dict[str, str]]] @overload @@ -132,17 +133,15 @@ def get_first(self, property: str) -> Union[None, str, vobject.vcard.Name, str. :param property: the field value to get - :param default: the value to return if the vCard does not have this - property - :returns: the property value or the default + :returns: the property value or None """ try: return getattr(self.vcard, property).value except AttributeError: return None - def _get_multi_property(self, name: str) -> list: - """Get a vCard property that can exist more than once. + def get_all(self, name: str) -> list: + """Get all values of the given vCard property. It does not matter what the individual vcard properties store as their value. This function returns them untouched inside an aggregating @@ -154,14 +153,12 @@ def _get_multi_property(self, name: str) -> list: :param name: the name of the property (should be UPPER case) :returns: the values from all occurrences of the named property """ - values = [] - for child in self.vcard.getChildren(): - if child.name == name: - ablabel = self._get_ablabel(child) - if ablabel: - values.append({ablabel: child.value}) - else: - values.append(child.value) + try: + values = getattr(self.vcard, f"{name.lower()}_list") + except AttributeError: + return [] + values = [{label: item.value} if (label := self._get_ablabel(item)) + else item.value for item in values] return sorted(values, key=multi_property_key) def _delete_vcard_object(self, name: str) -> None: @@ -585,12 +582,16 @@ def get_last_name_first_name(self) -> str: return self.formatted_name @property - def first_name(self) -> str: - return list_to_string(self._get_first_names(), " ") + def first_name(self) -> Optional[str]: + if parts := self._get_first_names(): + return list_to_string(parts, " ") + return None @property - def last_name(self) -> str: - return list_to_string(self._get_last_names(), " ") + def last_name(self) -> Optional[str]: + if parts := self._get_last_names(): + return list_to_string(parts, " ") + return None def _add_name(self, prefix: StrList, first_name: StrList, additional_name: StrList, last_name: StrList, @@ -617,7 +618,7 @@ def organisations(self) -> list[Union[list[str], dict[str, list[str]]]]: """ :returns: list of organisations, sorted alphabetically """ - return self._get_multi_property("ORG") + return self.get_all("org") def _add_organisation(self, organisation: StrList, label: Optional[str] = None) -> None: """Add one ORG entry to the underlying vcard @@ -640,48 +641,47 @@ def _add_organisation(self, organisation: StrList, label: Optional[str] = None) showas_obj.value = "COMPANY" @property - def titles(self) -> list[Union[str, dict[str, str]]]: - return self._get_multi_property("TITLE") + def titles(self) -> LabeledStrs: + return self.get_all("title") def _add_title(self, title: str, label: Optional[str] = None) -> None: self._add_labelled_property("title", title, label, True) @property - def roles(self) -> list[Union[str, dict[str, str]]]: - return self._get_multi_property("ROLE") + def roles(self) -> LabeledStrs: + return self.get_all("role") def _add_role(self, role: str, label: Optional[str] = None) -> None: self._add_labelled_property("role", role, label, True) @property - def nicknames(self) -> list[Union[str, dict[str, str]]]: - return self._get_multi_property("NICKNAME") + def nicknames(self) -> LabeledStrs: + return self.get_all("nickname") def _add_nickname(self, nickname: str, label: Optional[str] = None) -> None: self._add_labelled_property("nickname", nickname, label, True) @property - def notes(self) -> list[Union[str, dict[str, str]]]: - return self._get_multi_property("NOTE") + def notes(self) -> LabeledStrs: + return self.get_all("note") def _add_note(self, note: str, label: Optional[str] = None) -> None: self._add_labelled_property("note", note, label, True) @property - def webpages(self) -> list[Union[str, dict[str, str]]]: - return self._get_multi_property("URL") + def webpages(self) -> LabeledStrs: + return self.get_all("url") def _add_webpage(self, webpage: str, label: Optional[str] = None) -> None: self._add_labelled_property("url", webpage, label, True) @property def categories(self) -> Union[list[str], list[list[str]]]: - category_list = [] - for child in self.vcard.getChildren(): - if child.name == "CATEGORIES": - value = child.value - category_list.append( - value if isinstance(value, list) else [value]) + category_list = self.get_all("categories") + if not category_list: + return category_list + category_list = [value if isinstance(value, list) else [value] for + value in category_list] if len(category_list) == 1: return category_list[0] return sorted(category_list) @@ -766,13 +766,16 @@ def emails(self) -> dict[str, list[str]]: :returns: dict of type and email address list """ email_dict: dict[str, list[str]] = {} - for child in self.vcard.getChildren(): - if child.name == "EMAIL": - type = list_to_string( - self._get_types_for_vcard_object(child, "internet"), ", ") - if type not in email_dict: - email_dict[type] = [] - email_dict[type].append(child.value) + try: + emails = self.vcard.email_list + except AttributeError: + return {} + for child in emails: + type = list_to_string( + self._get_types_for_vcard_object(child, "internet"), ", ") + if type not in email_dict: + email_dict[type] = [] + email_dict[type].append(child.value) # sort email address lists for email_list in email_dict.values(): email_list.sort() @@ -817,19 +820,22 @@ def post_addresses(self) -> dict[str, list[PostAddress]]: :returns: dict of type and post address list """ post_adr_dict: dict[str, list[PostAddress]] = {} - for child in self.vcard.getChildren(): - if child.name == "ADR": - type = list_to_string(self._get_types_for_vcard_object( - child, "home"), ", ") - if type not in post_adr_dict: - post_adr_dict[type] = [] - post_adr_dict[type].append({"box": child.value.box, - "extended": child.value.extended, - "street": child.value.street, - "code": child.value.code, - "city": child.value.city, - "region": child.value.region, - "country": child.value.country}) + try: + addresses = self.vcard.adr_list + except AttributeError: + return {} + for child in addresses: + type = list_to_string(self._get_types_for_vcard_object( + child, "home"), ", ") + if type not in post_adr_dict: + post_adr_dict[type] = [] + post_adr_dict[type].append({"box": child.value.box, + "extended": child.value.extended, + "street": child.value.street, + "code": child.value.code, + "city": child.value.city, + "region": child.value.region, + "country": child.value.country}) # sort post address lists for post_adr_list in post_adr_dict.values(): post_adr_list.sort(key=lambda x: ( @@ -942,9 +948,9 @@ def __init__(self, vcard: vobject.base.Component, # getters and setters ##################### - def _get_private_objects(self) -> dict[str, list[Union[str, dict[str, str]]]]: + def _get_private_objects(self) -> dict[str, LabeledStrs]: supported = [x.lower() for x in self.supported_private_objects] - private_objects: dict[str, list[Union[str, dict[str, str]]]] = {} + private_objects: dict[str, LabeledStrs] = {} for child in self.vcard.getChildren(): lower = child.name.lower() if lower.startswith("x-") and lower[2:] in supported: diff --git a/test/test_vcard_wrapper.py b/test/test_vcard_wrapper.py index 266e9b5..9a4df64 100644 --- a/test/test_vcard_wrapper.py +++ b/test/test_vcard_wrapper.py @@ -510,7 +510,7 @@ class AddLabelledObject(unittest.TestCase): def assertTitle(self, expected): wrapper = TestVCardWrapper() yield wrapper - self.assertEqual(wrapper._get_multi_property("TITLE"), expected) + self.assertEqual(wrapper.get_all("title"), expected) def test_add_a_string(self): with self.assertTitle(["foo"]) as wrapper: @@ -578,10 +578,21 @@ def test_can_return_any_value_contradicting_type_annotation(self): class NullableProperties(unittest.TestCase): "test that properties that are not present on the vcard return None" + LIST_PROPERTIES = ["categories", "titles", "webpages", "organisations", + "notes", "roles", "nicknames"] + DICT_PROPERTIES = ["post_addresses", "emails", "phone_numbers"] + BASE_PROPERTIES = ["formatted_name", "kind", "version"] + def test_properties(self): for version in ["3.0", "4.0"]: card = TestVCardWrapper(version=version) for property in Contact.get_properties(): - if property not in ["formatted_name", "version", "kind"]: + if property in self.DICT_PROPERTIES: + with self.subTest(property=property, version=version): + self.assertEqual(getattr(card, property), {}) + elif property in self.LIST_PROPERTIES: + with self.subTest(property=property, version=version): + self.assertEqual(getattr(card, property), []) + elif property not in self.BASE_PROPERTIES: with self.subTest(property=property, version=version): self.assertIsNone(getattr(card, property))