Command line interface: Implement --fields for list command #246

earboxer · 2020-03-22T01:23:49Z

#179 (comment)

Things you can do with this

python3 -m khard ls -F name,birthday (like khard bdays, but it doesn't sort, doesn't exclude missing birthdays, and outputs in a different format)
python3 -m khard ls -pF emails.home.0,name (shows first home email address for each user)
python3 -m khard ls --fields phone_numbers.cell.0,phone_numbers (shows first cell number, and all phone numbers in python format)
python3 -m khard ls -p -F Name,post_addresses.home.0.region,post_addresses.home.0.code (name, state and zip code)

The list of properties can be seen with

grep '@property' -A1 khard/carddav_object.py

(Additional fields can easily be added by making them attributes in carddav_object.py)

For backwards compatibility with previous usage, the additional fields are defined:

index (counts 1,2,3,...)
name (formats name as desired)
~~address_book~~
phone
email
uid (prints minimized when not using --parsable)
~~uid_short~~

earboxer · 2020-03-22T01:34:07Z

This doesn't refactor any of the existing commands (I'm not sure what our deprecation strategy is for them).

This doesn't break existing functionality of list command.

This doesn't add a way to access first_name. (I'm not sure how we want to design the property for names)

lucc · 2020-03-22T01:37:31Z

Thank you that looks very cool! Can you add a proper doc comment to all your functions and check pep8 formatting, please (I think I saw some function arguments wothout spaces inbetween).

I will do a full review next.

earboxer · 2020-03-22T01:52:40Z

What do you think about the pretty_name function? It exists for the purpose of backwards compatibility (and maybe for the header to look nice?) It also exists so that the fields can be case insensitive... but maybe we would prefer to show the snake_cased ugly name instead?

lucc

I left some line comments. For pep8 there are also tools like pylint that you can use.

Nice work so far :)

khard/khard.py

lucc · 2020-03-22T02:00:26Z

khard/khard.py

+                attr_name = field.split('.')[0]
+                val = ''
+                if hasattr(vcard, attr_name):
+                    val = getattr(vcard, attr_name)
+                    # Nest multiple keys like emails.home.1
+                    for partial in field.split('.')[1:]:
+                        if isinstance(val, dict) and partial in val:
+                            val = val[partial]
+                        elif partial.isdigit() and isinstance(val, list) \
+                                and len(val) > int(partial):
+                            val = val[int(partial)]
+                        # TODO: Completely support case insensitive indexing
+                        elif isinstance(val, dict) and partial.upper() in val:
+                            val = val[partial.upper()]
+                        else:
+                            val = ''
+                row.append(val)


Maybe you could put this in a separate function get_nested_field. That would make it easier to write unit tests for this as it seems like an important piece of new code. What do you think?

Done. In what file should the tests be written for this function?

I think you could start a new test class in test/test_command_line_interface.py which is named after the function and has tests for some important cases of its behaviour.

Sounds like a good idea.

This merge request is getting pretty big though. I don't know when/if I'll get around to writing comprehensive tests for that function.

lucc · 2020-03-22T02:08:01Z

The idea of pretty_name and ugly_name looks good at first but there are some edge cases that do not look nice. Try khard ls -F n-a-----m-e. Maybe it is better to only use alphabetic characters in field names and normalize them with str.lower.

lucc

Please give your type documentation explicitly instead of inline in the text. Many other functions do it that way:

def foo(x, y):
    """This does the foo for bar to baz ...
    
    :param list(str|bool) x: the x that will be fooed
    :param y: that is the bar (with a long type name)
    :type y: some_long_module.and_type_name.Bar
    :returns: 42, always
    :rtype: int
    """
    return 42

lucc · 2020-03-22T02:09:06Z

khard/khard.py

@@ -1030,6 +1034,8 @@ def list_subcommand(vcard_list, parsable, fields):
    :type vcard_list: list of carddav_object.CarddavObject
    :param parsable: machine readable output: columns devided by tabulator (\t)
    :type parsable: bool
+    :param fields: list of strings for field evaluation
+    :type fields: list


Is this used to generate some documentation somewhere?

Otherwise, this type of repetitious documentation is not helping anyone;
would make the writers of https://www.python.org/dev/peps/pep-0257 sad.

Please update CONTRIBUTING.rst
if this type of documentation is necessary.

(otherwise, feel free to add it yourself).

These are used in the sphinx generated documentation. Try make -c doc html && $BROWSER doc/build/html/index.html locally or go to https://khard.readthedocs.io/ . Although the python modules (and these signatures) are not yet visible on readthedocs (only after the next release).

I tend to put a more "human" description in the :param: part and the explicit type signature in the :type: part.

Suggested change

:type fields: list

:param fields: the object path to the field on the vcard that should be printed

:type fields: list(str)

But I can fix these small things when I merge. I know that I am very picky about such things.

About duplication info: I am also thinking about real python type signatures in #204. But that is another big project and seems to bring its own complications.

earboxer · 2020-03-22T02:27:07Z

The idea of pretty_name and ugly_name looks good at first but there are some edge cases that do not look nice. Try khard ls -F n-a-----m-e. Maybe it is better to only use alphabetic characters in field names and normalize them with str.lower.

So then, should the "pretty" formats be TitleCase? (If the header reads "E-mail" as it does, then if someone sees that and wants to use khard -F E-mail, it wouldn't work). This would be a backwards incompatible change (though maybe those only matter for --parsable).

earboxer · 2020-03-22T02:50:27Z

Thank you for all the prompt feedback!

earboxer · 2020-03-22T03:18:57Z

Maybe it is better to only use alphabetic characters in field names and normalize them with str.lower.

Another consideration is that it is pulling attribute names from carddav_object, of which, many have underscores

version
uid
birthday
anniversary
formatted_name
organisations
titles
roles
nicknames
notes
webpages
categories
phone_numbers
emails
post_addresses

lucc · 2020-03-22T12:00:49Z

Is there any special reason you invited my to your khard repository?

I was thinking about normalizing field names and spotting typos:

I suggest that we allow only a very minimal character set in field names for the -F option. Maybe only [a-z] or [a-z_]. But I suggest not to allow -. I see that the header line currently says "E-Mail" but Wikipedia for example spells it Email. And I think that the field names in the header line are only read by humans and not programs. The -p format is modeled after mutt and they don't read the first line at all. They assume it is headers and skip it.

So if a user specifies invalid characters we can give an error message. But what about field names? Try khard ls -F namee. That will result in a bunch of empty lines which is ugly. I have two ideas:

If the fields for a matching contact result in only empty strings (so that the line would be empty) we could skip that contact in the output. So khard ls -F email foo would only output the email address of contacts matching foo and having an email to display. On the other hand khard ls -F name,email foo would output all names and emails of contacts matching foo that have at least a name or an email to show.
We could manage a list of valid fields (derived from the CarddavObject class maybe) and reject any other (top level) fields (nested fields might be harder).

Furthermore I was thinking about private object and other special cases:

I think we can postpone private objects and formatted post addresses and formatted anniversary and birthdays. But I came up with a plan that might be doable after this PR: Add new read only properties to the CarddavObject for these special cases (and other special cases like the different name gettes) so that they can be handled in a uniform way when formating output with fields. Later these can also be used for a custom query syntax like planned in #131.

lucc · 2020-03-24T06:12:18Z

Is there a reason why you have a markdown list token in front of every commit? I would remove them.

lucc · 2020-03-24T06:14:30Z

I like the idea about a fixed list of valid fields more the longer I think about it. We could still ignore case but require one specific spelling (plural or singular, underscores) and given propper error messages.

earboxer · 2020-03-25T03:21:19Z

Is there a reason why you have a markdown list token in front of every commit? I would remove them.

When I'm committing to a repository that doesn't have a commit message standard, I just go with what I'm used to (what we use at my workplace, which I believe was inspired by Code::Blocks's standard)

-/* System Name: Describe what it does in imperative mood

- means minor commit (documentation, one-line bugfix) * means major commit.

Personally, I don't care much for 'minor commits', and think in general you should make them go away when you git rebase before the merge.

earboxer · 2020-03-25T03:32:58Z

Right now uid appears differently if you use parsable than if you did not.

It is currently the only field that does so (and special logic was written so it does so).

Should we have a specific way of referring to the shortened uid, like uid_short, so that either option can be available in both parsable and non-parsable ways?

earboxer · 2020-03-25T03:44:31Z

It should be noted that 44c090d broke the existing tests...

lucc

I do not understand the reason for a6c2e25 . Why do we need the explicit name?

khard/khard.py

lucc · 2020-03-25T04:00:02Z

I would ask you to remove the * and - at the beginning of your commit messages. They might make sense if the people reading them know the policy but that is not (yet? :) the policy for this repository so they carry no information for most readers.

The category or section name in the beginning og the commit message is nice and can also be understood by new readers without extra documentation.

lucc · 2020-03-25T04:11:45Z

Currently None is printed for the birthday field if no birthday is set on the contact. This is in contrast to some/all? other fields which print the empty string. I would suggest to make it the empty string for all fields unless there are good arguments for None.

Aditionally we should think about two things related to empty fields (one was already mentioned above):

What happens with a contact that has all requested fields empty? I suggest to drop that contact (it would just produce an empty line).
Should we add some special field modifier to require a value in that field? Something like khard ls --fields 'name,!birthday' 'name,birthday! or any other char we deem acceptable. This would drop any contact from the output that does not have that field set. This would allow us to implement khard bdays as khard ls --fields 'name,birthday!'. An argument against this is that it mixes output logic with search term logic and such a constraint might better be represented in a custom query syntax like proposed in Search within specific fields #131.

earboxer · 2020-03-26T00:31:46Z

Currently None is printed for the birthday field if no birthday is set on the contact. This is in contrast to some/all? other fields which print the empty string. I would suggest to make it the empty string for all fields unless there are good arguments for None.

It's just doing python string conversion: the attribute is returning None. We should probably change the str() method on the value returned so it shows only the day, or the empty string.

What happens with a contact that has all requested fields empty? I suggest to drop that contact (it would just produce an empty line).

khard ls -F phone_numbers.cell | grep '\S'

Show only lines that have non-space-characters. (We would definitely want our None and {} to print as empty string in that case)

khard ls --fields 'name,birthday!'

khard ls --parsable --fields name,birthday | grep -Pv '\tNone'

-P for perl regex (allows the \t to mean tab), -v for inverse.

If we change it to output the empty string instead of 'None', this would be `grep -Pv '\t$' ($ meaning the end of the line).

I believe this project should only be as complicated as necessary...
khard show and khard edit should also reflect the attributes of the vcard object (so the fieldnames are consistent throughout). -- maybe the attributes of the vcard object should change to match khard show... (which would encourage delaying the merge/release of this feature/branch)

earboxer · 2020-03-26T00:54:08Z

I do not understand the reason for a6c2e25 . Why do we need the explicit name?

Was probably a little too overzealous making breaking changes. -- the idea was that the presentation of a field should not change when you change the -p flag (what if you wanted the short uid in a tab-separated output?)

Thinking about it more, it's probably not worth it for having the header change.

reverted in 98b395b

I think the only breaking chance you are authorizing is changing

'E-Mail' to 'Email'
'UID' to 'Uid'

earboxer · 2020-03-26T01:09:00Z

For a web-based project, I have been thinking about Edit-List-View for different fields of a model. In khard, this is pretty similar (except that the command for view is called show, and we have multiple lists for pseudo-models email, postaddress, phone).

Really, khard edit and khard show should be identical, except for the comments provided by edit

lucc

nearly done :)

The only things still missing are:

the failing tests and
the rewriting of the commit messages.
and the None problem and the field checking on the command line. I pushed some suggestions for that to the branch earboxer/khard@ls-fields...scheibler:ls-fields-suggestions You could use them or do something similar.

After that I think we can merge. 🎉

khard/khard.py

earboxer · 2020-03-27T04:36:15Z

Okay, I fixed the tests, and rewrote history a little.

lucc · 2020-03-27T06:59:39Z

Thank you very much @earboxer :)

lucc added enhancement interface changes the user interface in a non trivial way labels Mar 22, 2020

lucc requested changes Mar 22, 2020

View reviewed changes

lucc requested changes Mar 25, 2020

View reviewed changes

khard/khard.py Outdated Show resolved Hide resolved

lucc mentioned this pull request Mar 25, 2020

Redesigning the command line interface #179

Open

3 tasks

lucc requested changes Mar 26, 2020

View reviewed changes

khard/khard.py Show resolved Hide resolved

earboxer added 5 commits March 27, 2020 00:16

Tests: Add test for --fields argument in List Command

1c34151

List Command: Pass argument to list_contacts function

650e738

List Command: Allow specifying fields

b10aa5a

List Command: Make parsable option share the same code

fd64388

List Command: Enable multiple nesting

9ff3c02

earboxer and others added 7 commits March 27, 2020 00:19

List command changes: Add docstrings to new functions

8be1b7f

List Command Header: Use simple Title Case instead of prettification

511f52d

Check the type of the new --fields option thoroughly

bb1abcd

Field Checking: Fix typos

b8238ba

Use the empty string instead of None for missing values

c7098d7

Small formatting fix

f720cd6

Fix tests for List Command title case

6a3a0a5

earboxer force-pushed the ls-fields branch from 98b395b to 6a3a0a5 Compare March 27, 2020 04:28

lucc merged commit bf7c195 into lucc:develop Mar 27, 2020

lucc mentioned this pull request Mar 30, 2020

Please add automerge feature #248

Closed

lucc mentioned this pull request Aug 1, 2020

Improve zsh completion for field names #270

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command line interface: Implement --fields for list command #246

Command line interface: Implement --fields for list command #246

earboxer commented Mar 22, 2020 •

edited

Loading

earboxer commented Mar 22, 2020

lucc commented Mar 22, 2020

earboxer commented Mar 22, 2020

lucc left a comment

lucc Mar 22, 2020

earboxer Mar 22, 2020

lucc Mar 22, 2020

earboxer Mar 25, 2020

lucc commented Mar 22, 2020

lucc left a comment

lucc Mar 22, 2020

earboxer Mar 22, 2020 •

edited

Loading

lucc Mar 22, 2020

earboxer commented Mar 22, 2020

earboxer commented Mar 22, 2020

earboxer commented Mar 22, 2020

lucc commented Mar 22, 2020

lucc commented Mar 24, 2020

lucc commented Mar 24, 2020

earboxer commented Mar 25, 2020 •

edited

Loading

earboxer commented Mar 25, 2020

earboxer commented Mar 25, 2020

lucc left a comment

lucc commented Mar 25, 2020

lucc commented Mar 25, 2020

earboxer commented Mar 26, 2020

earboxer commented Mar 26, 2020

earboxer commented Mar 26, 2020

lucc left a comment

earboxer commented Mar 27, 2020

lucc commented Mar 27, 2020

	:type fields: list
	:param fields: the object path to the field on the vcard that should be printed
	:type fields: list(str)

Command line interface: Implement --fields for list command #246

Command line interface: Implement --fields for list command #246

Conversation

earboxer commented Mar 22, 2020 • edited Loading

earboxer commented Mar 22, 2020

lucc commented Mar 22, 2020

earboxer commented Mar 22, 2020

lucc left a comment

Choose a reason for hiding this comment

lucc Mar 22, 2020

Choose a reason for hiding this comment

earboxer Mar 22, 2020

Choose a reason for hiding this comment

lucc Mar 22, 2020

Choose a reason for hiding this comment

earboxer Mar 25, 2020

Choose a reason for hiding this comment

lucc commented Mar 22, 2020

lucc left a comment

Choose a reason for hiding this comment

lucc Mar 22, 2020

Choose a reason for hiding this comment

earboxer Mar 22, 2020 • edited Loading

Choose a reason for hiding this comment

lucc Mar 22, 2020

Choose a reason for hiding this comment

earboxer commented Mar 22, 2020

earboxer commented Mar 22, 2020

earboxer commented Mar 22, 2020

lucc commented Mar 22, 2020

lucc commented Mar 24, 2020

lucc commented Mar 24, 2020

earboxer commented Mar 25, 2020 • edited Loading

earboxer commented Mar 25, 2020

earboxer commented Mar 25, 2020

lucc left a comment

Choose a reason for hiding this comment

lucc commented Mar 25, 2020

lucc commented Mar 25, 2020

earboxer commented Mar 26, 2020

earboxer commented Mar 26, 2020

earboxer commented Mar 26, 2020

lucc left a comment

Choose a reason for hiding this comment

earboxer commented Mar 27, 2020

lucc commented Mar 27, 2020

earboxer commented Mar 22, 2020 •

edited

Loading

earboxer Mar 22, 2020 •

edited

Loading

earboxer commented Mar 25, 2020 •

edited

Loading