contact delete and list added #78

diveshuttam · 2018-07-03T16:21:50Z

closes #35 closes #40

dashohoxha · 2018-07-03T17:45:03Z

src/cmd/contact/delete.sh

-        gpg --delete-keys "$@"
-    else
-        gpg --batch --no-tty --yes --delete-keys "$@"
+        yesno "Delete contact(s). Are you sure?" || return 0


This is does not preserve the original behavior.
The question itself is meaningless and does not add any extra protection, without showing the contacts that are going to be deleted.

sourabhtk37

👍

Make tests pass

dashohoxha

What is the purpose of the files __init.py__?

dashohoxha · 2018-07-04T19:12:09Z

src/egpg.sh

@@ -124,6 +124,7 @@ call_fn() {
 call_gpg() {
    local file=$1; shift
    local pyfile="$LIBDIR/gpg/$file"
+    export PYTHONPATH=$PYTHONPATH:"$LIBDIR/gpg/"


This line should be better moved down, before the python3 call.

dashohoxha · 2018-07-04T19:19:02Z

src/gpg/contact/delete.py

+            for key in keys:
+                if(not force):
+                    print_key(key.fpr)
+                    ans = input("Delete this key from the keyring? (y/N)")


We are deleting contacts, not keys. Why do you print a key? Why do you ask for a key?

I am currently somewhat imitating what egpg did earlier. This is one of the ways it could be done. I pushed it here to discuss more about it.

So here is what gpg commands (egpg) originally did:

If I pass a egpg delete contact1 without --force

For each key of the contact gpg asked if you want to delete the key (with the exact same message)

it exited as soon as one key was deleted

If I pass egpg delete contact1 --force

it deletes all keys matching contact1

What i am doing right now is:

delete all keys matching the contact in case of --force without any messages

for all matching keys with the contact ask one by one and let the user choose to delete or not (y/N) (I feel it is helpful if he wants to delete specific keys corresponding to a contact name).

Would like your suggestions.

Another way to approach this could be:

by default doing a cmd_contact_list contact1 first and asking delete all these keys matching contact1 (y/N). If these are more keys let the user him/herself delete these by using keyids instead.

we might add a option --interactive which has the current behavior as mentioned in the above comment.

--force dominates over --interactive

I am currently somewhat imitating what egpg did earlier.

You can stick to the gpg behavior if you want. But I don't think it is right to delete single subkeys of a given contact (in case he has different encryption subkeys).

Actually I think that we are talking about different contacts, although they may have the same name and the same email address. So what you are doing may be right. You are actually deleting all the matching contacts (the contacts that match with the given search string), not all the keys of the given contact. I am not sure if I am explaining myself clearly.

When we talk about contact1, as an argument to the delete command, it can be an email address, or a full name, or a first name, etc. So, it is actually a search pattern which may match more than one contact (called key in the gpg language, which is an overloaded term according to me). It makes sense to ask for each of these matching contacts whether they should be deleted or not. But when we delete a contact, we delete it completely, not only one subkey of it. I believe this is also the current behavior of gpg.

Yup I think I got it : a contact in egpg is identified as a key (with subkeys) in gpg / gpgme.

I am deleting all subkeys with the key at once only.
I was actually talking about the case of multiple contact matches occur corresponding to the string passed.

I had borrowed the message Delete this key from the keyring? (y/N) from gpg only. If it seems confusing I can change key to contact.
It does delete a key with all subkeys together as mentioned above.

Yes, it should be confusing for the user, because as far as the user knows there is only one key, his own private key. All the rest are contacts.

dashohoxha · 2018-07-04T19:25:58Z

Sorry, I just notice the label "work in progress". Otherwise I would not have made a review.

print_key modified

diveshuttam · 2018-07-04T20:26:53Z

The purpose of __init__.py here is to allow imports like from fn.print_key import print_key etc.
It is basically used to tell python that the directory contains python modules and packages.

diveshuttam · 2018-07-04T20:27:13Z

@dashohoxha the PR can be reviewed now.

diveshuttam · 2018-07-04T20:30:55Z

Since this required quite some changes with the print_key functionality I also addressed contact list in this PR only.

some minor changes in contact list

diveshuttam · 2018-07-04T21:22:06Z

Sorry, I just notice the label "work in progress". Otherwise I would not have made a review.

Its fine and I am sorry too, I hadn't updated my progress for the day so pushed whatever I had done by that time.

dashohoxha · 2018-07-04T21:45:23Z

The purpose of init.py here is to allow imports like from fn.print_key import print_key etc.
It is basically used to tell python that the directory contains python modules and packages.

Now I understand it. I guess that you can merge this PR as soon as @sourabhtk37 reviews the code style etc.

requested changes addressed.

sourabhtk37

Just a few pointers here and there. Great job 👏

sourabhtk37 · 2018-07-05T05:50:28Z

src/gpg/contact/delete.py

+            keys = list(c.keylist(contact))
+            ans = "n"
+            for key in keys:
+                if(not force):


Why still use brackets for if expr? Please keep it consistent.

cool I have addressed this and also opened #79 also for this. This gets missed as I am using flake8 which doesn't detect this by default.

sourabhtk37 · 2018-07-05T05:52:07Z

src/gpg/contact/list.py

+    try:
+        c = gpg.Context()
+
+        key_set = set({})


Simply initialise a set as set().

sourabhtk37 · 2018-07-05T05:56:08Z

src/gpg/contact/list.py

+        key_set = set({})
+
+        for contact in contact_list:
+            key_set.update(set(c.keylist(contact)))


I think you might want to use the set.add() method or are we missing some use-case?

A contact (contact string) can have multiple matches. This is intended to list all of them.

Set is required as in case of multiple arguments, like egpg contact ls divesh $MYKEYID i want to list each contact only once.

if c.keylist returns a list, you don't need to convert it into a set is what I'm inferring, it will do that on its own afaik.

It returns a generator but yup it will do the job I guess.

sourabhtk37 · 2018-07-05T06:02:05Z

src/gpg/contact/list.py

+from fn.print_key import print_key
+
+
+def ls(contact_list):


Maybe have a bit more descriptive function name like: list_contact, you can do away with comments with this.
Tip: Your code should be such that it doesn't required comments to describe what it is doing. I think it's pretty clear with the function name.
Something to read:https://blog.codinghorror.com/coding-without-comments/

sourabhtk37

@dashohoxha Good to go imo.

contact delete added

f057b68

diveshuttam added this to the Second Evaluations milestone Jul 3, 2018

diveshuttam self-assigned this Jul 3, 2018

diveshuttam requested review from dashohoxha and sourabhtk37 July 3, 2018 16:21

dashohoxha previously requested changes Jul 3, 2018

View reviewed changes

sourabhtk37 reviewed Jul 3, 2018

View reviewed changes

diveshuttam added 2 commits July 4, 2018 23:52

Modify contact delete behaviour

abfd5ba

modify formatting for print key

acf6841

diveshuttam changed the title ~~contact delete added~~ [WIP:DNM] contact delete added Jul 4, 2018

diveshuttam added the work in progress label Jul 4, 2018

Correct force functionality

15ec2ff

Make tests pass

dashohoxha reviewed Jul 4, 2018

View reviewed changes

Key list functionality added

90ae8f6

print_key modified

diveshuttam removed the work in progress label Jul 4, 2018

diveshuttam changed the title ~~[WIP:DNM] contact delete added~~ contact delete and list added Jul 4, 2018

diveshuttam added 2 commits July 5, 2018 01:28

move export PYTHONPATH down

4becb0b

refine delete

1816d55

diveshuttam added 2 commits July 5, 2018 02:25

changes for PEP8

5825c9d

some minor changes in contact list

preserve exit behavior on EOF

4c229ee

Make message simpler for the user.

b22b0f1

diveshuttam force-pushed the issue35 branch from 40359e9 to b22b0f1 Compare July 4, 2018 22:07

sourabhtk37 reviewed Jul 5, 2018

View reviewed changes

address some stylistic issues

75d1ba8

sourabhtk37 approved these changes Jul 5, 2018

View reviewed changes

diveshuttam merged commit a3a74de into gnupg-2.2 Jul 5, 2018

diveshuttam deleted the issue35 branch July 6, 2018 03:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contact delete and list added #78

contact delete and list added #78

diveshuttam commented Jul 3, 2018 •

edited

Loading

dashohoxha Jul 3, 2018

sourabhtk37 left a comment

dashohoxha left a comment

dashohoxha Jul 4, 2018

diveshuttam Jul 4, 2018

dashohoxha Jul 4, 2018

diveshuttam Jul 4, 2018 •

edited

Loading

diveshuttam Jul 4, 2018 •

edited

Loading

dashohoxha Jul 4, 2018

dashohoxha Jul 4, 2018

diveshuttam Jul 4, 2018

diveshuttam Jul 4, 2018 •

edited

Loading

dashohoxha Jul 4, 2018

dashohoxha commented Jul 4, 2018

diveshuttam commented Jul 4, 2018 •

edited

Loading

diveshuttam commented Jul 4, 2018

diveshuttam commented Jul 4, 2018 •

edited

Loading

diveshuttam commented Jul 4, 2018

dashohoxha commented Jul 4, 2018

sourabhtk37 left a comment

sourabhtk37 Jul 5, 2018

diveshuttam Jul 5, 2018 •

edited

Loading

sourabhtk37 Jul 5, 2018

sourabhtk37 Jul 5, 2018

diveshuttam Jul 5, 2018

diveshuttam Jul 5, 2018

sourabhtk37 Jul 5, 2018

diveshuttam Jul 5, 2018

sourabhtk37 Jul 5, 2018

sourabhtk37 left a comment

contact delete and list added #78

contact delete and list added #78

Conversation

diveshuttam commented Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

sourabhtk37 left a comment

Choose a reason for hiding this comment

dashohoxha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diveshuttam Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

diveshuttam Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diveshuttam Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashohoxha commented Jul 4, 2018

diveshuttam commented Jul 4, 2018 • edited Loading

diveshuttam commented Jul 4, 2018

diveshuttam commented Jul 4, 2018 • edited Loading

diveshuttam commented Jul 4, 2018

dashohoxha commented Jul 4, 2018

sourabhtk37 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diveshuttam Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourabhtk37 left a comment

Choose a reason for hiding this comment

diveshuttam commented Jul 3, 2018 •

edited

Loading

diveshuttam Jul 4, 2018 •

edited

Loading

diveshuttam Jul 4, 2018 •

edited

Loading

diveshuttam Jul 4, 2018 •

edited

Loading

diveshuttam commented Jul 4, 2018 •

edited

Loading

diveshuttam commented Jul 4, 2018 •

edited

Loading

diveshuttam Jul 5, 2018 •

edited

Loading