From 2e3bd4795d6844ed0db94733552656d7e43bdf49 Mon Sep 17 00:00:00 2001 From: Lucas Hoffmann Date: Fri, 20 Dec 2024 19:51:21 +0100 Subject: [PATCH 1/5] Add tests for nullable properties --- test/test_vcard_wrapper.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/test/test_vcard_wrapper.py b/test/test_vcard_wrapper.py index 19d98f3..bc8c7ea 100644 --- a/test/test_vcard_wrapper.py +++ b/test/test_vcard_wrapper.py @@ -8,7 +8,7 @@ import vobject -from khard.contacts import VCardWrapper +from khard.contacts import Contact, VCardWrapper from khard.helpers.typing import ObjectType from .helpers import vCard, TestVCardWrapper @@ -574,3 +574,15 @@ def test_can_return_any_value_contradicting_type_annotation(self): p.value = vobject.vcard.Name(family='Foo', given='Bar') self.assertEqual(wrapper.get_first("n"), vobject.vcard.Name(family='Foo', given='Bar')) + + +class NullableProperties(unittest.TestCase): + "test that properties that are not present on the vcard return None" + + 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"]: + with self.subTest(property=property, version=version): + self.assertIsNone(getattr(card, property)) From 3c07f986dabe2d0c5f4026160911f27fb8e5b53f Mon Sep 17 00:00:00 2001 From: Lucas Hoffmann Date: Tue, 12 Dec 2023 06:47:22 +0100 Subject: [PATCH 2/5] Make get_first nullable --- khard/contacts.py | 32 +++++++++++++++++++++++--------- khard/khard.py | 6 +++--- test/test_query.py | 4 ---- test/test_vcard_wrapper.py | 3 +-- 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/khard/contacts.py b/khard/contacts.py index 159cda4..b74b26a 100644 --- a/khard/contacts.py +++ b/khard/contacts.py @@ -112,12 +112,19 @@ def __init__(self, vcard: vobject.base.Component, def __str__(self) -> str: return self.formatted_name - def get_first(self, property: str, default: str = "") -> str: + @overload + def get_first(self, property: Literal["n"]) -> Optional[vobject.vcard.Name]: ... + @overload + def get_first(self, property: Literal["adr"]) -> Optional[vobject.vcard.Address]: ... + @overload + def get_first(self, property: str) -> Optional[str]: ... + def get_first(self, property: str) -> Union[None, str, vobject.vcard.Name, + vobject.vcard.Address]: """Get a property from the underlying vCard. This method should only be called for properties with cardinality \\*1 (zero or one). Otherwise only the first value will be returned. If the - property is not present a default will be returned. + property is not present None will be retuned. The type annotation for the return value is str but this is not enforced so it is up to the caller to make sure to only call this @@ -132,7 +139,7 @@ def get_first(self, property: str, default: str = "") -> str: try: return getattr(self.vcard, property).value except AttributeError: - return default + return None def _get_multi_property(self, name: str) -> list: """Get a vCard property that can exist more than once. @@ -259,7 +266,7 @@ def _get_types_for_vcard_object(self, object: vobject.base.ContentLine, return [default_type] @property - def version(self) -> str: + def version(self) -> Optional[str]: return self.get_first("version") @version.setter @@ -274,7 +281,7 @@ def version(self, value: str) -> None: version.value = convert_to_vcard("version", value, ObjectType.str) @property - def uid(self) -> str: + def uid(self) -> Optional[str]: return self.get_first("uid") @uid.setter @@ -470,7 +477,7 @@ def _prepare_birthday_value(self, date: Date) -> tuple[Optional[str], @property def kind(self) -> str: - return self.get_first(self._kind_attribute_name().lower(), self._default_kind) + return self.get_first(self._kind_attribute_name().lower()) or self._default_kind @kind.setter def kind(self, value: str) -> None: @@ -487,10 +494,15 @@ def _kind_attribute_name(self) -> str: @property def formatted_name(self) -> str: - return self.get_first("fn") + fn = self.get_first("fn") + if fn: + return fn + self.formatted_name = "" + return self.get_first("fn") or "" @formatted_name.setter def formatted_name(self, value: str) -> None: + # TODO cardinality 1* """Set the FN field to the new value. All previously existing FN fields are deleted. Version 4 of the specs @@ -1303,9 +1315,11 @@ def to_yaml(self) -> str: "Note": self.notes, "Webpage": self.webpages, "Anniversary": - helpers.yaml_anniversary(self.anniversary, self.version), + helpers.yaml_anniversary(self.anniversary, self.version or + self._default_version), "Birthday": - helpers.yaml_anniversary(self.birthday, self.version), + helpers.yaml_anniversary(self.birthday, self.version or + self._default_version), "Address": helpers.yaml_addresses( self.post_addresses, ["Box", "Extended", "Street", "Code", "City", "Region", "Country"], defaults=["home"]) diff --git a/khard/khard.py b/khard/khard.py index 109defc..bc5bc24 100644 --- a/khard/khard.py +++ b/khard/khard.py @@ -210,9 +210,9 @@ def list_contacts(vcard_list: list[Contact], fields: Iterable[str] = (), row.append(formatter.get_special_field(vcard, field)) elif field == 'uid': if parsable: - row.append(vcard.uid) - elif abook_collection.get_short_uid(vcard.uid): - row.append(abook_collection.get_short_uid(vcard.uid)) + row.append(vcard.uid or "") + elif uid := abook_collection.get_short_uid(vcard.uid or ""): + row.append(uid) else: row.append("") else: diff --git a/test/test_query.py b/test/test_query.py index f1d45bd..5af43c7 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -105,11 +105,7 @@ def test_and_queries_match_after_sorting(self): class TestFieldQuery(unittest.TestCase): - @unittest.expectedFailure def test_empty_field_values_match_if_the_field_is_present(self): - # This test currently fails because the Contact class has all - # attributes set because they are properties. So the test in the query - # class if an attribute is present never fails. uid = "Some Test Uid" vcard1 = TestContact(uid=uid) vcard2 = TestContact() diff --git a/test/test_vcard_wrapper.py b/test/test_vcard_wrapper.py index bc8c7ea..266e9b5 100644 --- a/test/test_vcard_wrapper.py +++ b/test/test_vcard_wrapper.py @@ -564,8 +564,7 @@ def test_get_only_the_first_property(self): def test_returns_the_default(self): wrapper = TestVCardWrapper() - self.assertEqual(wrapper.get_first("title"), "") - self.assertEqual(wrapper.get_first("title", "foo"), "foo") + self.assertIsNone(wrapper.get_first("title")) def test_can_return_any_value_contradicting_type_annotation(self): """This is discouraged!""" From 4be3cb2571ba6042ed9349e593016dbf0d2af178 Mon Sep 17 00:00:00 2001 From: Lucas Hoffmann Date: Mon, 23 Dec 2024 10:17:08 +0100 Subject: [PATCH 3/5] 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)) From ffdb103bd3236c457c22a6200871c69768832eb3 Mon Sep 17 00:00:00 2001 From: Lucas Hoffmann Date: Wed, 25 Dec 2024 16:00:22 +0100 Subject: [PATCH 4/5] Add failing test to distinguish empty N from no N --- khard/contacts.py | 7 +++---- test/test_vcard_wrapper.py | 16 ++++++++++++++-- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/khard/contacts.py b/khard/contacts.py index c74819d..9752bba 100644 --- a/khard/contacts.py +++ b/khard/contacts.py @@ -531,10 +531,9 @@ def _get_names_part(self, part: str) -> list[str]: the_list = getattr(self.vcard.n.value, part) except AttributeError: return [] - else: - # check if list only contains empty strings - if not ''.join(the_list): - return [] + # check if list only contains empty strings + if not ''.join(the_list): + return [] return the_list if isinstance(the_list, list) else [the_list] def _get_name_prefixes(self) -> list[str]: diff --git a/test/test_vcard_wrapper.py b/test/test_vcard_wrapper.py index 9a4df64..79938cd 100644 --- a/test/test_vcard_wrapper.py +++ b/test/test_vcard_wrapper.py @@ -576,14 +576,15 @@ 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" + "test that attributes 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): + def test_for_non_existing_attributes(self): + """Non existing attributes""" for version in ["3.0", "4.0"]: card = TestVCardWrapper(version=version) for property in Contact.get_properties(): @@ -596,3 +597,14 @@ def test_properties(self): elif property not in self.BASE_PROPERTIES: with self.subTest(property=property, version=version): self.assertIsNone(getattr(card, property)) + + @unittest.expectedFailure + def test_no_name_is_not_equal_to_empty_name(self): + # FIXME this fails because khard.contacts.VCardWrapper._get_names_part + # specifically treats a name where all components are the empty string + # the same way as no N attribute at all. + empty = TestVCardWrapper() + empty._add_name("", "", "", "", "") + noname = TestVCardWrapper() + self.assertNotEqual(empty.first_name, noname.first_name) + self.assertNotEqual(empty.last_name, noname.last_name) From a756186b35b186790509d5fbafde98da6fd282b5 Mon Sep 17 00:00:00 2001 From: Lucas Hoffmann Date: Wed, 25 Dec 2024 16:14:42 +0100 Subject: [PATCH 5/5] Add more tests for existence queries --- test/test_query.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/test/test_query.py b/test/test_query.py index 5af43c7..6b0133e 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -105,7 +105,7 @@ def test_and_queries_match_after_sorting(self): class TestFieldQuery(unittest.TestCase): - def test_empty_field_values_match_if_the_field_is_present(self): + def test_empty_field_values_match_if_sstring_field_is_present(self): uid = "Some Test Uid" vcard1 = TestContact(uid=uid) vcard2 = TestContact() @@ -113,6 +113,20 @@ def test_empty_field_values_match_if_the_field_is_present(self): self.assertTrue(query.match(vcard1)) self.assertFalse(query.match(vcard2)) + def test_empty_field_values_match_if_list_field_is_present(self): + vcard1 = TestContact(categories=["foo", "bar"]) + vcard2 = TestContact() + query = FieldQuery("categories", "") + self.assertTrue(query.match(vcard1)) + self.assertFalse(query.match(vcard2)) + + def test_empty_field_values_match_if_dict_field_is_present(self): + query = FieldQuery("emails", "") + vcard = TestContact() + self.assertFalse(query.match(vcard)) + vcard.add_email("home", "a@b.c") + self.assertTrue(query.match(vcard)) + def test_empty_field_values_fails_if_the_field_is_absent(self): vcard = TestContact() query = FieldQuery("emails", "")