[PATCH 2/2] DNFPayload: install DNF itself.

Ales Kozumplik akozumpl at redhat.com
Mon Oct 7 08:38:50 UTC 2013


On 10/04/2013 07:14 PM, Brian C. Lane wrote:
> Does the order matter? If not, I'd rather see this added to
> self._required_pkgs at some point, either in preInstall or maybe as a
> default and then change preInstall to append to the list.

self._required_pkgs only exists to store the packages passed to 
preInstall() until install() is called. It is a minor flaw of the 
Payload interface itself that these two methods are not one since they 
are always called in succession (see install.py:161), but the DNFPayload 
has to respect that and the most straightforward way by far to pass the 
list on to nstall() is to store it in self._required_pkgs. That's the 
only reason why this instance variable exists and it might go away in 
the future. In no way would appending to it or even merging it with a 
new special instance variable, say _internally_required_pkgs result in a 
cleaner design and better maintainability than the single call to 
_install_package('dnf'), which is what I stress the most right after 
functionality.

>
> I also notice that some of the variable naming is different from yumpayload.py
>
> For example, dnf is using:
>
> self._required_groups = []
> self._required_pkgs = []

In Python, using underscore at the instance member start is used to 
indicate internal use in the instance. It does not really matter to the 
outisde world (like yumpayload.py) how such variable is called, it is 
part of the implementation details. Following the same name of a similar 
variable of different interface implementation can only be useful if the 
interface guaranteed its existence, which is hard to see for a private 
variable, or alternatively to breach the implementation encapsulation by 
the interface clients, which is definitely not desirable in this case, 
if ever.

Second, it can not be denied Anaconda is using both camelCase and 
underscore_convention in the code. Even the YumPayload itself is guilty 
of this:

   self._repos_dir = 
"/etc/yum.repos.d,/etc/anaconda.repos.d,/tmp/updates/anaconda.repos.d,/tmp/product/anaconda.repos.d"
   ...
   self._requiredPackages = []

If Anaconda settled for one I would have no problem to follow the 
convention. Until then I decided to use underscores as I perceive it as 
more common in the current Python ecosystem.

Third, YumPayload will eventually get obsoleted and removed, so it makes 
little sense to be making the new module adhere to the old one in any 
way except the public interface.

> It would be nice if we could use the same names where the functionality
> is the same between the packaging modules.

I perceive your comments to this trivial patch as somewhat bully-ish. 
Notice also that because of the time gap between us I will not be able 
to push this at least the following 24 hours. This is not really helping 
as the process of developing for Anaconda for (now) an outsider is 
already quite difficult as it is.

Ales



More information about the anaconda-patches mailing list