Add kind column in contact table #309

afh · 2022-06-12T09:09:20Z

This is a first, likely incomplete, attempt to add a kind column in [the] contact table as mentioned in the todo.txt.

I haven't been able to get the tests running with my nixpkgs setup on macOS due to errors (see below). Being unfamiliar with mypy any comment that might be helpful is appreciated.

% nix-shell -p nsh python3 python3Packages.unidecode python3Packages.ruamel-yaml python3Packages.vobject python3Packages.atomicwrites python3Packages.configobj python3Packages.setuptools python3Packages.pip pylint mypy --run "mypy khard"

khard/carddav_object.py:20: error: Library stubs not installed for "atomicwrites" (or incompatible with Python 3.9)
khard/carddav_object.py:20: note: Hint: "python3 -m pip install types-atomicwrites"
khard/carddav_object.py:20: note: (or run "mypy --install-types" to install all missing stub packages)
khard/carddav_object.py:23: error: Skipping analyzing "vobject": module is installed, but missing library stubs or py.typed marker
khard/address_book.py:10: error: Skipping analyzing "vobject.base": module is installed, but missing library stubs or py.typed marker
khard/address_book.py:10: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
khard/address_book.py:10: error: Skipping analyzing "vobject": module is installed, but missing library stubs or py.typed marker
khard/config.py:11: error: Skipping analyzing "configobj": module is installed, but missing library stubs or py.typed marker
khard/config.py:16: error: Skipping analyzing "validate": module is installed, but missing library stubs or py.typed marker
khard/khard.py:288: error: Argument 1 to "append" of "list" has incompatible type "methodcaller"; expected "attrgetter[Any]"
khard/khard.py:290: error: Argument 1 to "append" of "list" has incompatible type "methodcaller"; expected "attrgetter[Any]"
khard/khard.py:445: error: Incompatible types in assignment (expression has type "TermQuery", variable has type "AnyQuery")
khard/khard.py:448: error: Incompatible types in assignment (expression has type "AndQuery", variable has type "AnyQuery")
Found 10 errors in 4 files (checked 14 source files)

afh · 2022-06-12T09:10:13Z

I probably should have made this a draft pull request…

lucc · 2022-06-12T10:21:35Z

mypy and tests

The tests are also broken on the develop branch, I am working on it in #307. Some of your errors are already fixed in the github-actions branch but when you use nix you have to build a python env with the python packages you want, just installing them side by side does not work. Check https://nixos.org/manual/nixpkgs/stable/#python

With nix-shell --pure -p python3.withPackages' (p: with p;[mypy ruamel_yaml vobject configobj unidecode])' --run "mypy khard" I still get

khard/carddav_object.py:23: error: Skipping analyzing 'vobject': found module but no type hints or library stubs
khard/carddav_object.py:1275: error: Argument 1 to "yaml_clean" has incompatible type "object"; expected "Union[str, Sequence[Any], Dict[str, Any], None]"
khard/address_book.py:10: error: Skipping analyzing 'vobject.base': found module but no type hints or library stubs
khard/address_book.py:10: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
khard/address_book.py:10: error: Skipping analyzing 'vobject': found module but no type hints or library stubs
khard/config.py:11: error: Skipping analyzing 'configobj': found module but no type hints or library stubs
khard/config.py:16: error: Skipping analyzing 'validate': found module but no type hints or library stubs
Found 6 errors in 3 files (checked 14 source files)

but with --ignore-missing-imports it is only

khard/carddav_object.py:1275: error: Argument 1 to "yaml_clean" has incompatible type "object"; expected "Union[str, Sequence[Any], Dict[str, Any], None]"
Found 1 error in 1 file (checked 14 source files)

your PR

Firstly: Thank you :)

I think you do not need a config option "preferred_kind" as the standard says

Implementations MUST support the specific string values defined
above. If this property is absent, "individual" MUST be assumed
as the default.

And if a config option would actually have been needed it needs to be added to the config specification file.

Even if some upstream tests are broken you can still implement tests for your new code.

afh · 2022-06-15T15:25:21Z

Thanks for your feedback, I'll work it in.
Regarding tests it seemed to me that tests did not run at all due to the errors I saw, yet that could well be my unfamiliarity with mypy; I'll look into it.

lucc · 2022-06-15T16:37:35Z

mypy does not run the tests. It is only a type checker. Use python setup.py test to run the tests. You can also have a look at the travis config file for the exact commands that would be run in CI.

Remove kind setter as it is not relevant for displaying the kind attribute.

afh · 2022-06-17T08:55:56Z

Thanks for your helpful explanations, @lucc. I went ahead and fixed the tests in regards to the newly added kind column being displayed.

My preference would be to merge this PR once you find it acceptable and work on the kind editing and filtering in a separate PR (#310), but I'm happy to do what you think works best for you.

ℹ️ I'm uncertain whether the following is an issue only with my setup, yet the test_contact_is_found_if_name_matches (test.test_command_line_interface.AddEmail) ... test hangs, i.e. is unresponsive and does not finish; I let it "run" for a minute and decided then to ^C it…

lucc · 2022-06-20T06:22:20Z

I am unsure if we should display the kind attribute by default. My reasoning goes like this:

I assume that most users would have only "individual" vcards in their address books
"individual" is the default if no kind attribute is present on the vcard. This means we add noise to the output if the attribute is not set.
if only "individial" vcards are in a listing it adds very little info to display this.

I have two ideas to improve this:

a config/command line option to show the kind
a similar logic like with the address book column: only show it if not all cards are "individual" (the address book column is only shown when the output contains cards from more than one address book but I would also show the kind if all cards in the output have the same kind but that kind is not "individual")

What do you think?

khard/formatter.py

afh · 2022-06-20T15:56:13Z

I think your idea to improve displaying the kind column conditionally and via a config/command-line option makes sense. I'll look into it. Thanks for the suggestion :)

when result list contains contacts different kinds.

afh · 2022-06-25T14:33:30Z

khard/carddav_object.py

+    @property
+    def kind(self) -> str:
+        kind = self._get_string_field("kind") or self._default_kind
+        return kind if kind != "org" else "organisation"


I'm uncertain whether org should be displayed as organisation…
khard requires to use organisations when querying for the organisation. Yet the corresponding value of the KIND property is defined as org (see RFC 6350 6.1.4 KIND) and the RFC always spells it as organization (note the use of z instead of s).

What are your thoughts on this @lucc

That seems to be the difference between British and American English: https://en.wikipedia.org/wiki/Organization

I am not a native speaker and don't know what we have chosen in the rest of khard (I just greped from color and colour but there are no matches for either).

I think we we will take whichever and wait for somebody to complain.

Sticking with the British spelling as khard already uses "organisation" rather than "organization" seems to a good solution for the time being then.

when result list contains contacts with non-individual kinds

to configure whether the kind column should always be shown

afh · 2022-06-25T15:00:53Z

@lucc, this should be ready for another look.
I went ahead and added a new config option (show_kinds similar to show_uids) allowing to configure whether the kind column should always be shown (defaults to no) as well as displaying the kind column conditionally whether the result list consists entirely of or contains contacts with non-individual kind.

lucc

This looks good, just the test files seem to be missing.

lucc · 2022-06-27T05:59:37Z

khard/carddav_object.py

+    @property
+    def kind(self) -> str:
+        kind = self._get_string_field("kind") or self._default_kind
+        return kind if kind != "org" else "organisation"


That seems to be the difference between British and American English: https://en.wikipedia.org/wiki/Organization

I am not a native speaker and don't know what we have chosen in the rest of khard (I just greped from color and colour but there are no matches for either).

I think we we will take whichever and wait for somebody to complain.

lucc · 2022-06-27T06:06:20Z

test/test_command_line_interface.py

@@ -210,6 +210,27 @@ def test_postaddr_lists_only_contacts_with_post_addresses(self):
                  '                             SomeState, HomeCountry']
        self.assertListEqual(expect, text)

+    def test_mixed_kinds(self):
+        with TmpConfig(["org.vcf", "individual.vcf"]):


Did you commit these files? I don't see them.

I forgot to add them… 🤦‍♂️

lucc · 2022-06-27T20:26:13Z

Lets move on to #310 :)

afh · 2022-06-28T09:21:56Z

Thanks for merging, @lucc 😃

Add kind column in contact table

a6d215a

afh mentioned this pull request Jun 12, 2022

Allow querying by kind #310

Merged

lucc added the enhancement label Jun 12, 2022

afh added 2 commits June 13, 2022 12:43

Merge branch 'develop' into afh-kind-column

5b01687

Replace _preferred_kind in _default_kind

58582b2

afh added 2 commits June 17, 2022 09:48

Merge branch 'develop' into afh-kind-column

fe58edd

Fix tests related to showing kind attribute

13468f7

Remove kind setter as it is not relevant for displaying the kind attribute.

lucc reviewed Jun 20, 2022

View reviewed changes

khard/formatter.py Outdated Show resolved Hide resolved

afh added 6 commits June 25, 2022 09:47

Merge branch 'develop' into afh-kind-column

45fddf4

Conditionally display kind column

d16ce03

when result list contains contacts different kinds.

Remove kind for default table header

7a30463

Remove unneeded formatter._default_kind

0159f53

Fix tests

bfc1398

Fix tests, take two

6658704

afh commented Jun 25, 2022

View reviewed changes

afh added 2 commits June 25, 2022 14:43

Conditionally display kind column

d0ec0b0

when result list contains contacts with non-individual kinds

Add show_kinds config option

41fb8f8

to configure whether the kind column should always be shown

Add show_kinds test

de38f85

lucc requested changes Jun 27, 2022

View reviewed changes

Add missing files

cd3e2d5

lucc merged commit 2791c40 into lucc:develop Jun 27, 2022

afh deleted the afh-kind-column branch June 28, 2022 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kind column in contact table #309

Add kind column in contact table #309

afh commented Jun 12, 2022 •

edited

Loading

afh commented Jun 12, 2022

lucc commented Jun 12, 2022

afh commented Jun 15, 2022

lucc commented Jun 15, 2022

afh commented Jun 17, 2022

lucc commented Jun 20, 2022

afh commented Jun 20, 2022

afh Jun 25, 2022

lucc Jun 27, 2022

afh Jun 27, 2022

afh commented Jun 25, 2022

lucc left a comment

lucc Jun 27, 2022

lucc Jun 27, 2022

afh Jun 27, 2022

lucc commented Jun 27, 2022

afh commented Jun 28, 2022

Add kind column in contact table #309

Add kind column in contact table #309

Conversation

afh commented Jun 12, 2022 • edited Loading

afh commented Jun 12, 2022

lucc commented Jun 12, 2022

mypy and tests

your PR

afh commented Jun 15, 2022

lucc commented Jun 15, 2022

afh commented Jun 17, 2022

lucc commented Jun 20, 2022

afh commented Jun 20, 2022

afh Jun 25, 2022

Choose a reason for hiding this comment

lucc Jun 27, 2022

Choose a reason for hiding this comment

afh Jun 27, 2022

Choose a reason for hiding this comment

afh commented Jun 25, 2022

lucc left a comment

Choose a reason for hiding this comment

lucc Jun 27, 2022

Choose a reason for hiding this comment

lucc Jun 27, 2022

Choose a reason for hiding this comment

afh Jun 27, 2022

Choose a reason for hiding this comment

lucc commented Jun 27, 2022

afh commented Jun 28, 2022

afh commented Jun 12, 2022 •

edited

Loading