Re: [devel] [PATCH for apt] Implemented generic callback system for package manager transactions

From: Aleksei Nikiforov <darktemplar@altlinux.org>
To: devel@lists.altlinux.org
Subject: Re: [devel] [PATCH for apt] Implemented generic callback system for package manager transactions
Date: Tue, 10 Dec 2019 10:37:02 +0300
Message-ID: <8690dc9e-0cdf-be87-5cbe-b7940347415b@altlinux.org> (raw)
In-Reply-To: <20191210003042.GF15867@altlinux.org>

10.12.2019 3:30, Dmitry V. Levin пишет:
> On Fri, Dec 06, 2019 at 04:12:01PM +0300, Oleg Solovyov wrote:
>> ---
>>   apt/apt-pkg/packagemanager.cc |   4 +-
>>   apt/apt-pkg/packagemanager.h  |  30 +++++++-
>>   apt/apt-pkg/rpm/rpmpm.cc      | 137 ++++++++++++++++++++++++++++++++--
>>   apt/apt-pkg/rpm/rpmpm.h       |  16 ++--
>>   4 files changed, 170 insertions(+), 17 deletions(-)
> 
> I agree the code should speak for itself, but it would be great
> if you could shed some light on what's going on.
> 
> [...]
>> +   aptCallbackType callbackType = APTCALLBACK_UNKNOWN;
>> +   switch (what) {
>> +      case RPMCALLBACK_INST_PROGRESS:
>> +         callbackType = APTCALLBACK_INST_PROGRESS;
>> +         break;
>> +      case RPMCALLBACK_INST_START:
>> +         callbackType = APTCALLBACK_INST_START;
>> +         break;
>> +      case RPMCALLBACK_TRANS_PROGRESS:
>> +         callbackType = APTCALLBACK_TRANS_PROGRESS;
>> +         break;
>> +      case RPMCALLBACK_TRANS_START:
>> +         callbackType = APTCALLBACK_TRANS_START;
>> +         break;
>> +      case RPMCALLBACK_TRANS_STOP:
>> +         callbackType = APTCALLBACK_TRANS_STOP;
>> +         break;
>> +      case RPMCALLBACK_UNINST_PROGRESS:
>> +         callbackType = APTCALLBACK_UNINST_PROGRESS;
>> +         break;
>> +      case RPMCALLBACK_UNINST_START:
>> +         callbackType = APTCALLBACK_UNINST_START;
>> +         break;
>> +      case RPMCALLBACK_UNINST_STOP:
>> +         callbackType = APTCALLBACK_UNINST_STOP;
>> +         break;
>> +      case RPMCALLBACK_UNPACK_ERROR:
>> +         callbackType = APTCALLBACK_UNPACK_ERROR;
>> +         break;
>> +      case RPMCALLBACK_CPIO_ERROR:
>> +         callbackType = APTCALLBACK_CPIO_ERROR;
>> +         break;
>> +      case RPMCALLBACK_SCRIPT_ERROR:
>> +         callbackType = APTCALLBACK_SCRIPT_ERROR;
>> +         break;
>> +      case RPMCALLBACK_SCRIPT_START:
>> +         callbackType = APTCALLBACK_SCRIPT_START;
>> +         break;
>> +      case RPMCALLBACK_SCRIPT_STOP:
>> +         callbackType = APTCALLBACK_SCRIPT_STOP;
>> +         break;
>> +      case RPMCALLBACK_INST_STOP:
>> +         callbackType = APTCALLBACK_INST_STOP;
>> +         break;
>> +      case RPMCALLBACK_ELEM_PROGRESS:
>> +         callbackType = APTCALLBACK_ELEM_PROGRESS;
>> +         break;
>> +      default:
>> +         break;
>> +   }
> 
> This looks ugly.  Could we use the same values for corresponding
> APTCALLBACK_* and RPMCALLBACK_* constants instead?
> 
> 

Not all RPMCALLBACK_* values are present in APTCALLBACK_* by design. How 
would you treat 'default' case?

Before this change, APT was hiding RPM interfaces from APT users. This 
enum was introduced to keep RPM interface hidden. How do you intend to 
have same constants in different enum? If you propose something like 
'APTCALLBACK_UNKNOWN = RPMCALLBACK_UNKNOWN', it'd couple APT interface 
with RPM interface. And hardcoding same values is more ugly than 
switch/case in my opinion.

> 
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
> 