Open afh opened 2 years ago
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.
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?
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.
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.
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?
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.
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?
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.
Friendly ping / nudge requesting re-review of the latest changes, @lucc đ
Sorry for the delay. I had another look:
Can you also rebase this PR on the current develop branch so the list of commits is cleaned up.
Thanks for your feedback @lucc, much appreciated đ
I thought about this a bit more and with my current understanding of things I'd like to run the following by you, @lucc:
VCardWrapper
had its own validate
method, which calls validate
on its vcard
property.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?
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.
@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.
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.
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.