Code review comment for lp:~silhusk/software-properties/lp1656100-remove-signing-keys

Revision history for this message
Sebastien Bacher (seb128) wrote :

> Let's consider remove_key() defined at softwareproperties/SoftwareProperties.py:838, for instance.
> The call to self.KeysModified is wrapped in a try-except that catches the AttributeError exception
> (self doesn't have any KeysModified), and make remove_key always return False.

Right, I did try to remove the try/except and still don't see an error though, maybe I did test that wrongly, I'm going to give it another try

>> * shouldn't those calls be moved to the Dbus backend
> Yes, definitely (imho). But that's a different story.

Well, I was just speaking about moving the self.KeysModified() lines instead of removing them (same that was done in http://launchpadlibrarian.net/91837493/software-properties_0.82.3_0.82.4.diff.gz). Why do you consider it a "different story"? e.g RemoveKey() in SoftwarePropertiesDbus.py line 312 could have that line added no, that way the change signal keeps being emited? or do you think that's not required for some reason and if so could give some details on why you think those calls are not useful/can be purely removed?

> Using :16 was just the smallest change to make it work, with the added benefit of using 64 bits key id.

Right, that makes sense, that's fine with me. I would like to see the KeysModified question resolved before we merge the changes though.

review: Needs Information

« Back to merge proposal