Allow querying by kind

[afh]

This is a first, likely incomplete, attempt to add an option to filter contact table (--kind) as mentioned in the todo.txt.

ℹ️ This PR depends on changes introduced in #309 (merged). Please see #309 for details on why this PR does not (yet) contain tests.

Any comments on whether this is headed into the right direction or not is greatly appreciated.

[lucc]

The todo file is very old and we have since introduced a query syntax. I would prefer a new query key word kind: over the option --kind. This would also integrate effortlessly with all subcommands and not just the list command.

You could split the editing of the kind attribute via the yaml file into a separate commit and move it to #309. Then we can discuss just the query syntax stuff here. Or you can even move the whole code to #309 and call it "support the kind attribute" or so.

[afh]

Ah I see, the query syntax vs. options was not clear to me. I'll rework this PR accordingly.
I'm curious: What is the motivation to keep deprecated options around instead of introduction a new major version with breaking changes?

[lucc]

The motivation is that I simply did not make a release yet. And I was working with my private undocumented policy to not remove stuff unless it has been marked and documented as "deprecated" for at least one release.

[afh]

Thanks for the context, @lucc, on why the options are still around. I went ahead and reworked this PR so that it supports querying by kind.
Being still rather unfamiliar with khard's code I may have missed a few things. I'll dig into the code a bit more and see where some tests would be helpful. Appreciate if you could direct towards areas that may need more work in order to fully support the kind attribute.

[afh]

Do you feel that the existing tests around the kind column (i.e. test_mixed_kinds and test_non_individual_kind ) provide sufficient test-coverage for this PR to be merged, @lucc?

[lucc]

I did a first review.

There is one important question we have to decide early I think: How to handle "kind" with vcard v3. The rfc for v3 does not talk about kind.

I think we can either not support kind for v3 or we should check what the rfc says about custom attributes and then treat it as a custom field on v3 cards. (see the anniversary field for an example)

[lucc]

where did you get "application" and "device" from? I can not see them in the rfc

Suggested change

	# one of: individual, group, org, location, application, device
	# one of: individual, group, org, location

[afh]

ℹ️ "vCard KIND:application" is from RFC 6473 and "vCard KIND:device" from RFC 6869

[lucc]

I would reference these rfcs in that comment and also at the top of the file.

[lucc]

About the tests: I would hope for some more test that specifically edit the kind field. And depending on if we support kind on v3 we could have test for this case too.

[afh]

Thanks for the helpful review, @lucc; much appreciated. I went ahead and addressed your comments.

How should khard behave if a non-standard value is given for kind, e.g. person? Should it default to individual? Skip processing the kind attribute? Warn? Use the given value?

[afh]

Regarding KIND on 3.0 contacts I decided to take the same approach as khard takes with ANNIVERSARY and prefix it with X- to make it a private attribute.

[afh]

Friendly ping / nudge requesting re-review of the latest changes, @lucc 🙂

[lucc]

Sorry for the delay. I had another look:

• I checked the RFCs for KIND:application and KIND:device and I think it is ok to support them.
• Can you mention these RFCs in the carddav_object.py file at the top with the other rfcs
• if the vobject library validates the kind property we can just use that (check the birthday and anniversary tests for an example) otherwise I suggest do not allow any unsupported values to be set. We should emit a warning if someone tries. The biggest question is how to handle the case if we edit a vcard that already had an unsupported value for KIND and it was not changed by the user during editing?
• for some more unittests you can have a look how it is done for other attributes in test/test_vcard_wrapper.py
• for editing the kinnd attribute you could have a look at the tests in test/test_yaml.py (UpdateVcardWithYamlUserInput)

Can you also rebase this PR on the current develop branch so the list of commits is cleaned up.

[afh]

Thanks for your feedback @lucc, much appreciated 🙏

• Glad to hear that you think it is ok to support them 🙂
• I've added a brief comment and link to the respective RFCs
• Will look into how the vobject library handles things in the coming days
• Will add more unit tests in the coming days
• Will add more edit tests in the coming days
• Happy to rebase, yet this forces me to force push the changes onto this branch. Would you be open to squash this PR when merging?

[afh]

I thought about this a bit more and with my current understanding of things I'd like to run the following by you, @lucc:

• What if VCardWrapper had its own validate method, which calls validate on its vcard property.
• What if VCardWrapper remembered the attributes that were updated and uses that information in order to be smart about validating, e.g. skip validation of the kind attribute.

Would that kind of approach work? I haven't looked too much into vobject's validation implementation. Are you more knowledgable about it than me and could point me to interesting parts?

[lucc]

Having a separate validation logic on VCardWrapper might be an interesting idea. But I think we would have to discuss this in a separate issue.

For now I suggest we just check the value of the kind directly our self and do not bother to understand or wrap what vobject does in this regard. Like this we can get this PR done and discuss new ideas later on.

[lucc]

@afh are you still working on this? We (@scheibler and I) are planning to make the 0.18 release this year and I originally wanted to include this as we have merged #309.

I just had a look at this and added a kind column to one of my vcards and it did not show up with khard ed. I conclude that we need some more tests for this PR.

If you are working on this and want to help get it into the 0.18 release please also rebase on latest develop. Please also drop me a note if you can not work on it this month, then we will go forward with the release without this PR.

[afh]

Please go ahead with the 0.18 release excluding this PR. I'm uncertain if or when I'll find the time to continue on this PR. I've forgotten the details, yet I think the issue grew to wider circles, i.e. the vobject library.